[v3,1/8] ethdev: add member notification for bonding port

  From: Long Wu <long.wu@corigine.com>

Bonding PMD does not let member ports know the bonding port's
information, like how many member ports the bonding port has,
what mode the bonding port is in and so on.

Add the notification interface for bonding port to let member
port know it is added to a bonding port and what the bonding
port's configuration is. If so the member ports have chance to
achieve its bond-flow-offlod or other private bonding functions.

Signed-off-by: Long Wu <long.wu@corigine.com>
Reviewed-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/bonding/eth_bond_private.h |  1 +
 drivers/net/bonding/rte_eth_bond.h     | 46 ++++++++++++++++
 drivers/net/bonding/rte_eth_bond_api.c | 73 ++++++++++++++++++++++++++
 drivers/net/bonding/rte_eth_bond_pmd.c | 27 ++++++++--
 drivers/net/bonding/version.map        |  3 ++
 lib/ethdev/ethdev_driver.h             | 18 +++++++
 6 files changed, 165 insertions(+), 3 deletions(-)
  

Hi Chaoyong,

some comments as below.


在 2023/10/8 9:50, Chaoyong He 写道:
> From: Long Wu <long.wu@corigine.com>
>
> Bonding PMD does not let member ports know the bonding port's
> information, like how many member ports the bonding port has,
> what mode the bonding port is in and so on.
>
> Add the notification interface for bonding port to let member
> port know it is added to a bonding port and what the bonding
> port's configuration is. If so the member ports have chance to
> achieve its bond-flow-offlod or other private bonding functions.
"its bond-flow-offlod or other private bonding functions"
I wonder that what PMDs can do with this.
Can you give an example in PMD to help others review?
>
> Signed-off-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: James Hershaw <james.hershaw@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> ---
>   drivers/net/bonding/eth_bond_private.h |  1 +
>   drivers/net/bonding/rte_eth_bond.h     | 46 ++++++++++++++++
>   drivers/net/bonding/rte_eth_bond_api.c | 73 ++++++++++++++++++++++++++
>   drivers/net/bonding/rte_eth_bond_pmd.c | 27 ++++++++--
>   drivers/net/bonding/version.map        |  3 ++
>   lib/ethdev/ethdev_driver.h             | 18 +++++++
>   6 files changed, 165 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/bonding/eth_bond_private.h b/drivers/net/bonding/eth_bond_private.h
> index e688894210..f69e85c199 100644
> --- a/drivers/net/bonding/eth_bond_private.h
> +++ b/drivers/net/bonding/eth_bond_private.h
> @@ -180,6 +180,7 @@ struct bond_dev_private {
>   	uint8_t member_update_idx;
>   
>   	bool kvargs_processing_is_done;
> +	bool notify_member; /**< Enable member notification of bonding port. */
>   
>   	uint32_t candidate_max_rx_pktlen;
>   	uint32_t max_rx_pktlen;
> diff --git a/drivers/net/bonding/rte_eth_bond.h b/drivers/net/bonding/rte_eth_bond.h
> index f10165f2c6..737beca446 100644
> --- a/drivers/net/bonding/rte_eth_bond.h
> +++ b/drivers/net/bonding/rte_eth_bond.h
> @@ -351,6 +351,52 @@ rte_eth_bond_link_up_prop_delay_set(uint16_t bonding_port_id,
>   int
>   rte_eth_bond_link_up_prop_delay_get(uint16_t bonding_port_id);
>   
> +/**
> + * Set the flag of whether bonding port notifies member ports.
> + *
> + * @param bonding_port_id
> + *   Port ID of bonding device.
> + * @param notify_member
> + *   Flag of whether bonding port notifies member ports.
> + *
> + * @return
> + *   0 on success, negative value otherwise.
> + */
> +__rte_experimental
> +int
> +rte_eth_bond_notify_member_flag_set(uint16_t bonding_port_id, bool notify_member);
s/notify_membe/notify in input?
because function name reveals the meaning already.
> +
> +/**
> + * Get the flag of whether bonding port notifies member ports.
> + *
> + * @param bonding_port_id
> + *   Port ID of bonding device.
> + * @param notify_member
> + *   Flag of whether bonding port notifies member ports.
> + *
> + * @return
> + *   0 on success, negative value otherwise.
> + */
> +__rte_experimental
> +int
> +rte_eth_bond_notify_member_flag_get(uint16_t bonding_port_id, bool *notify_member);
> +
> +/**
> + * Notify the member ports of bonding port's information.
> + *
> + * This interface is called in the following functions:
> + * - bond_ethdev_lsc_event_callback()
> + * - bond_ethdev_configure()
Is this interface used just  in these cases?
If so I don't think it should be a API in eth_dev_op.
> + *
> + * @param bonding_port_id
> + *   Port ID of bonding device.
> + *
> + * @return
> + *   0 on success, negative value otherwise.
> + */
> +__rte_experimental
> +int
> +rte_eth_bond_notify_members(uint16_t bonding_port_id);
>   
>   #ifdef __cplusplus
>   }
> diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
> index 99e496556a..48c1d050fd 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -627,6 +627,17 @@ __eth_bond_member_add_lock_free(uint16_t bonding_port_id, uint16_t member_port_i
>   
>   	member_vlan_filter_set(bonding_port_id, member_port_id);
>   
> +	if (internals->notify_member &&
> +			*member_eth_dev->dev_ops->bond_notify_member != NULL) {
> +		ret = member_eth_dev->dev_ops->bond_notify_member(member_eth_dev,
> +				bonding_eth_dev);
> +		if (ret < 0) {
> +			RTE_BOND_LOG(ERR, "Add member (port %u) notify failed!",
> +					member_port_id);
> +			return -1;
> +		}
> +	}
> +
>   	return 0;
>   
>   }
> @@ -733,6 +744,10 @@ __eth_bond_member_remove_lock_free(uint16_t bonding_port_id,
>   	member_eth_dev = &rte_eth_devices[member_port_id];
>   	member_remove(internals, member_eth_dev);
>   	member_eth_dev->data->dev_flags &= (~RTE_ETH_DEV_BONDING_MEMBER);
> +	if (internals->notify_member &&
> +			*member_eth_dev->dev_ops->bond_notify_member != NULL)
> +		member_eth_dev->dev_ops->bond_notify_member(member_eth_dev,
> +				bonding_eth_dev);
>   
>   	/*  first member in the active list will be the primary by default,
>   	 *  otherwise use first device in list */
> @@ -1098,3 +1113,61 @@ rte_eth_bond_link_up_prop_delay_get(uint16_t bonding_port_id)
>   
>   	return internals->link_up_delay_ms;
>   }
> +
> +int
> +rte_eth_bond_notify_member_flag_set(uint16_t bonding_port_id, bool notify_member)
> +{
> +	struct bond_dev_private *internals;
> +
> +	if (valid_bonding_port_id(bonding_port_id) != 0)
> +		return -EINVAL;
> +
> +	internals = rte_eth_devices[bonding_port_id].data->dev_private;
> +
> +	internals->notify_member = notify_member;
> +
> +	return 0;
> +}
> +
> +int
> +rte_eth_bond_notify_member_flag_get(uint16_t bonding_port_id, bool *notify_member)
> +{
> +	struct bond_dev_private *internals;
> +
> +	if (valid_bonding_port_id(bonding_port_id) != 0)
> +		return -EINVAL;
> +
> +	internals = rte_eth_devices[bonding_port_id].data->dev_private;
> +
> +	*notify_member = internals->notify_member;
> +
> +	return 0;
> +}
> +
> +int
> +rte_eth_bond_notify_members(uint16_t bonding_port_id)
> +{
> +	uint32_t i;
> +	uint16_t member_port_id;
> +	struct rte_eth_dev *bond_dev;
> +	struct bond_dev_private *internals;
> +	struct rte_eth_dev *member_dev[RTE_MAX_ETHPORTS];
> +
> +	if (valid_bonding_port_id(bonding_port_id) != 0)
> +		return -EINVAL;
> +
> +	bond_dev = &rte_eth_devices[bonding_port_id];
> +	internals = bond_dev->data->dev_private;
> +
> +	for (i = 0; i < internals->member_count; i++) {
> +		member_port_id = internals->members[i].port_id;
> +		member_dev[i] = &rte_eth_devices[member_port_id];
> +		if (*member_dev[i]->dev_ops->bond_notify_member == NULL)
> +			return -ENOTSUP;
> +	}
Do we need all member ports support bond_notify_member api?
If allow user to select one member port to notify? This might be more 
general.
In addition, this action here is inconsistent with above handle(do not 
notify member if doesn't supportbond_notify_member API).


> +
> +	for (i = 0; i < internals->member_count; i++)
> +		member_dev[i]->dev_ops->bond_notify_member(member_dev[i], bond_dev);
> +
> +	return 0;
> +}
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 122b1187fd..b99b8b8938 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2968,11 +2968,13 @@ bond_ethdev_lsc_event_callback(uint16_t port_id, enum rte_eth_event_type type,
>   	int valid_member = 0;
>   	uint16_t active_pos, member_idx;
>   	uint16_t i;
> +	uint16_t bonding_port_id;
>   
>   	if (type != RTE_ETH_EVENT_INTR_LSC || param == NULL)
>   		return rc;
>   
> -	bonding_eth_dev = &rte_eth_devices[*(uint16_t *)param];
> +	bonding_port_id = *(uint16_t *)param;
> +	bonding_eth_dev = &rte_eth_devices[bonding_port_id];
>   
>   	if (check_for_bonding_ethdev(bonding_eth_dev))
>   		return rc;
> @@ -3044,8 +3046,10 @@ bond_ethdev_lsc_event_callback(uint16_t port_id, enum rte_eth_event_type type,
>   		 * using it.
>   		 */
>   		if (internals->user_defined_primary_port &&
> -				internals->primary_port == port_id)
> +				internals->primary_port == port_id) {
>   			bond_ethdev_primary_set(internals, port_id);
> +			rte_eth_bond_notify_members(bonding_port_id);
> +		}
>   	} else {
>   		if (active_pos == internals->active_member_count)
>   			goto link_update;
> @@ -3064,6 +3068,7 @@ bond_ethdev_lsc_event_callback(uint16_t port_id, enum rte_eth_event_type type,
>   						internals->active_members[0]);
>   			else
>   				internals->current_primary_port = internals->primary_port;
> +			rte_eth_bond_notify_members(bonding_port_id);
>   			mac_address_members_update(bonding_eth_dev);
>   			bond_ethdev_promiscuous_update(bonding_eth_dev);
>   			bond_ethdev_allmulticast_update(bonding_eth_dev);
> @@ -3362,6 +3367,7 @@ dump_basic(const struct rte_eth_dev *dev, FILE *f)
>   	struct bond_dev_private instant_priv;
>   	const struct bond_dev_private *internals = &instant_priv;
>   	int mode, i;
> +	bool notify_member;
>   
>   	/* Obtain a instance of dev_private to prevent data from being modified. */
>   	memcpy(&instant_priv, dev->data->dev_private, sizeof(struct bond_dev_private));
> @@ -3431,6 +3437,13 @@ dump_basic(const struct rte_eth_dev *dev, FILE *f)
>   		fprintf(f, "\tUser Defined Primary: [%u]\n", internals->primary_port);
>   	if (internals->member_count > 0)
>   		fprintf(f, "\tCurrent Primary: [%u]\n", internals->current_primary_port);
> +
> +	if (rte_eth_bond_notify_member_flag_get(internals->port_id, &notify_member) == 0)
> +		fprintf(f, "\tNotify Member Ports Flag: %s\n",
> +			notify_member ? "enable" : "disable");
> +	else
> +		fprintf(f, "\tFailed to get notify member ports flag for bonding port %d\n",
> +			internals->port_id);
>   }
>   
>   static void
> @@ -3983,8 +3996,12 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
>   	 * if no kvlist, it means that this bonding device has been created
>   	 * through the bonding api.
>   	 */
> -	if (!kvlist || internals->kvargs_processing_is_done)
> +	if (!kvlist || internals->kvargs_processing_is_done) {
> +		if (internals->notify_member && rte_eth_bond_notify_members(port_id) != 0)
> +			RTE_BOND_LOG(ERR, "Notify member ports failed");
> +
>   		return 0;
> +	}
>   
>   	internals->kvargs_processing_is_done = true;
>   
> @@ -4222,6 +4239,10 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
>   			return -1;
>   		}
>   	}
> +
> +	if (internals->notify_member && rte_eth_bond_notify_members(port_id) != 0)
> +		RTE_BOND_LOG(ERR, "Notify member ports failed");
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/net/bonding/version.map b/drivers/net/bonding/version.map
> index 09ee21c55f..3bd5e8ad11 100644
> --- a/drivers/net/bonding/version.map
> +++ b/drivers/net/bonding/version.map
> @@ -35,4 +35,7 @@ EXPERIMENTAL {
>   	rte_eth_bond_member_add;
>   	rte_eth_bond_member_remove;
>   	rte_eth_bond_members_get;
> +	rte_eth_bond_notify_member_flag_get;
> +	rte_eth_bond_notify_member_flag_set;
> +	rte_eth_bond_notify_members;
>   };
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index deb23ada18..f626f971e5 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1216,6 +1216,21 @@ typedef int (*eth_count_aggr_ports_t)(struct rte_eth_dev *dev);
>   typedef int (*eth_map_aggr_tx_affinity_t)(struct rte_eth_dev *dev, uint16_t tx_queue_id,
>   					  uint8_t affinity);
>   
> +/**
> + * @internal
> + * Bonding port notifies the member ports.
> + *
> + * @param dev
> + *   Member port (ethdev) handle.
> + * @param bonding_dev
> + *   Bonding port (ethdev) handle.
> + *
> + * @return
> + *   Negative on error, 0 on success.
> + */
> +typedef int (*eth_bond_notify_member)(struct rte_eth_dev *dev,
> +				      struct rte_eth_dev *bonding_dev);
> +
>   /**
>    * @internal A structure containing the functions exported by an Ethernet driver.
>    */
> @@ -1455,6 +1470,9 @@ struct eth_dev_ops {
>   	eth_count_aggr_ports_t count_aggr_ports;
>   	/** Map a Tx queue with an aggregated port of the DPDK port */
>   	eth_map_aggr_tx_affinity_t map_aggr_tx_affinity;
> +
> +	/** Notify the member port of bonding port information */
> +	eth_bond_notify_member bond_notify_member;
>   };
>   
>   /**
  

> Hi Chaoyong,
> 
> some comments as below.
> 
> 
> 在 2023/10/8 9:50, Chaoyong He 写道:
> > From: Long Wu <long.wu@corigine.com>
> >
> > Bonding PMD does not let member ports know the bonding port's 
> > information, like how many member ports the bonding port has, what 
> > mode the bonding port is in and so on.
> >
> > Add the notification interface for bonding port to let member port 
> > know it is added to a bonding port and what the bonding port's 
> > configuration is. If so the member ports have chance to achieve its 
> > bond-flow-offlod or other private bonding functions.
> "its bond-flow-offlod or other private bonding functions"
> I wonder that what PMDs can do with this.
> Can you give an example in PMD to help others review?

After this patch series, I will send out nfp PMD code about "its bond-flow-offlod or other private bonding functions ".
I can explain here:
"bond-flow" means the flow rule's destination port is a bonding port.
Now DPDK can use bonding port as the source port in a flow rule and member ports can offload this flow.
But member ports cannot offload a flow that its destination port is a bonding port.
Because the member ports don't know the bonding port. (Of course, some PMDs has its self-function to let member ports know the bonding port but it doesn't a general "DPDK" way).
After this "notify patch", DPDK can do "bond-flow-offload", also "other private bonding functions"(like hardware balance policy, primary port changing and so on) can work.

> >
> > Signed-off-by: Long Wu <long.wu@corigine.com>
> > Reviewed-by: James Hershaw <james.hershaw@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > ---
> >   drivers/net/bonding/eth_bond_private.h |  1 +
> >   drivers/net/bonding/rte_eth_bond.h     | 46 ++++++++++++++++
> >   drivers/net/bonding/rte_eth_bond_api.c | 73
> ++++++++++++++++++++++++++
> >   drivers/net/bonding/rte_eth_bond_pmd.c | 27 ++++++++--
> >   drivers/net/bonding/version.map        |  3 ++
> >   lib/ethdev/ethdev_driver.h             | 18 +++++++
> >   6 files changed, 165 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/bonding/eth_bond_private.h
> > b/drivers/net/bonding/eth_bond_private.h
> > index e688894210..f69e85c199 100644
> > --- a/drivers/net/bonding/eth_bond_private.h
> > +++ b/drivers/net/bonding/eth_bond_private.h
> > @@ -180,6 +180,7 @@ struct bond_dev_private {
> >   	uint8_t member_update_idx;
> >
> >   	bool kvargs_processing_is_done;
> > +	bool notify_member; /**< Enable member notification of bonding port.
> > +*/
> >
> >   	uint32_t candidate_max_rx_pktlen;
> >   	uint32_t max_rx_pktlen;
> > diff --git a/drivers/net/bonding/rte_eth_bond.h
> > b/drivers/net/bonding/rte_eth_bond.h
> > index f10165f2c6..737beca446 100644
> > --- a/drivers/net/bonding/rte_eth_bond.h
> > +++ b/drivers/net/bonding/rte_eth_bond.h
> > @@ -351,6 +351,52 @@ rte_eth_bond_link_up_prop_delay_set(uint16_t
> bonding_port_id,
> >   int
> >   rte_eth_bond_link_up_prop_delay_get(uint16_t bonding_port_id);
> >
> > +/**
> > + * Set the flag of whether bonding port notifies member ports.
> > + *
> > + * @param bonding_port_id
> > + *   Port ID of bonding device.
> > + * @param notify_member
> > + *   Flag of whether bonding port notifies member ports.
> > + *
> > + * @return
> > + *   0 on success, negative value otherwise.
> > + */
> > +__rte_experimental
> > +int
> > +rte_eth_bond_notify_member_flag_set(uint16_t bonding_port_id, bool 
> > +notify_member);
> s/notify_membe/notify in input?
> because function name reveals the meaning already.

Thank you, you are right.

> > +
> > +/**
> > + * Get the flag of whether bonding port notifies member ports.
> > + *
> > + * @param bonding_port_id
> > + *   Port ID of bonding device.
> > + * @param notify_member
> > + *   Flag of whether bonding port notifies member ports.
> > + *
> > + * @return
> > + *   0 on success, negative value otherwise.
> > + */
> > +__rte_experimental
> > +int
> > +rte_eth_bond_notify_member_flag_get(uint16_t bonding_port_id, bool 
> > +*notify_member);
> > +
> > +/**
> > + * Notify the member ports of bonding port's information.
> > + *
> > + * This interface is called in the following functions:
> > + * - bond_ethdev_lsc_event_callback()
> > + * - bond_ethdev_configure()
> Is this interface used just  in these cases?
> If so I don't think it should be a API in eth_dev_op.

Sorry I'm a bit confused.
Do you mean "rte_eth_bond_notify_members" this interface? This interface is called in 3 functions.

> > ...
> > +	struct rte_eth_dev *member_dev[RTE_MAX_ETHPORTS];
> > +
> > +	if (valid_bonding_port_id(bonding_port_id) != 0)
> > +		return -EINVAL;
> > +
> > +	bond_dev = &rte_eth_devices[bonding_port_id];
> > +	internals = bond_dev->data->dev_private;
> > +
> > +	for (i = 0; i < internals->member_count; i++) {
> > +		member_port_id = internals->members[i].port_id;
> > +		member_dev[i] = &rte_eth_devices[member_port_id];
> > +		if (*member_dev[i]->dev_ops->bond_notify_member == NULL)
> > +			return -ENOTSUP;
> > +	}
> Do we need all member ports support bond_notify_member api?
> If allow user to select one member port to notify? This might be more general.
> In addition, this action here is inconsistent with above handle(do not 
> notify member if doesn't supportbond_notify_member API).

Yes, we do not need all member ports support this API.
I just want to have a stricter restriction on this feature before, but I think your suggestion is better.

> > ...
  

在 2023/10/9 11:11, Long Wu 写道:
>> Hi Chaoyong,
>>
>> some comments as below.
>>
>>
>> 在 2023/10/8 9:50, Chaoyong He 写道:
>>> From: Long Wu <long.wu@corigine.com>
>>>
>>> Bonding PMD does not let member ports know the bonding port's
>>> information, like how many member ports the bonding port has, what
>>> mode the bonding port is in and so on.
>>>
>>> Add the notification interface for bonding port to let member port
>>> know it is added to a bonding port and what the bonding port's
>>> configuration is. If so the member ports have chance to achieve its
>>> bond-flow-offlod or other private bonding functions.
>> "its bond-flow-offlod or other private bonding functions"
>> I wonder that what PMDs can do with this.
>> Can you give an example in PMD to help others review?
> After this patch series, I will send out nfp PMD code about "its bond-flow-offlod or other private bonding functions ".
> I can explain here:
> "bond-flow" means the flow rule's destination port is a bonding port.
> Now DPDK can use bonding port as the source port in a flow rule and member ports can offload this flow.
> But member ports cannot offload a flow that its destination port is a bonding port.
> Because the member ports don't know the bonding port. (Of course, some PMDs has its self-function to let member ports know the bonding port but it doesn't a general "DPDK" way).
> After this "notify patch", DPDK can do "bond-flow-offload", also "other private bonding functions"(like hardware balance policy, primary port changing and so on) can work.
I think what you said is more like "bonding offload", right?
It seems that you cannot do it just based on current these API in this 
series.
You probably have other works to upstream for like "bond-flow" and 
"other private bonding functions".

>
>>> Signed-off-by: Long Wu <long.wu@corigine.com>
>>> Reviewed-by: James Hershaw <james.hershaw@corigine.com>
>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>> ---
>>>    drivers/net/bonding/eth_bond_private.h |  1 +
>>>    drivers/net/bonding/rte_eth_bond.h     | 46 ++++++++++++++++
>>>    drivers/net/bonding/rte_eth_bond_api.c | 73
>> ++++++++++++++++++++++++++
>>>    drivers/net/bonding/rte_eth_bond_pmd.c | 27 ++++++++--
>>>    drivers/net/bonding/version.map        |  3 ++
>>>    lib/ethdev/ethdev_driver.h             | 18 +++++++
>>>    6 files changed, 165 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/eth_bond_private.h
>>> b/drivers/net/bonding/eth_bond_private.h
>>> index e688894210..f69e85c199 100644
>>> --- a/drivers/net/bonding/eth_bond_private.h
>>> +++ b/drivers/net/bonding/eth_bond_private.h
>>> @@ -180,6 +180,7 @@ struct bond_dev_private {
>>>    	uint8_t member_update_idx;
>>>
>>>    	bool kvargs_processing_is_done;
>>> +	bool notify_member; /**< Enable member notification of bonding port.
>>> +*/
>>>
>>>    	uint32_t candidate_max_rx_pktlen;
>>>    	uint32_t max_rx_pktlen;
>>> diff --git a/drivers/net/bonding/rte_eth_bond.h
>>> b/drivers/net/bonding/rte_eth_bond.h
>>> index f10165f2c6..737beca446 100644
>>> --- a/drivers/net/bonding/rte_eth_bond.h
>>> +++ b/drivers/net/bonding/rte_eth_bond.h
>>> @@ -351,6 +351,52 @@ rte_eth_bond_link_up_prop_delay_set(uint16_t
>> bonding_port_id,
>>>    int
>>>    rte_eth_bond_link_up_prop_delay_get(uint16_t bonding_port_id);
>>>
>>> +/**
>>> + * Set the flag of whether bonding port notifies member ports.
>>> + *
>>> + * @param bonding_port_id
>>> + *   Port ID of bonding device.
>>> + * @param notify_member
>>> + *   Flag of whether bonding port notifies member ports.
>>> + *
>>> + * @return
>>> + *   0 on success, negative value otherwise.
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_eth_bond_notify_member_flag_set(uint16_t bonding_port_id, bool
>>> +notify_member);
>> s/notify_membe/notify in input?
>> because function name reveals the meaning already.
> Thank you, you are right.
>
>>> +
>>> +/**
>>> + * Get the flag of whether bonding port notifies member ports.
>>> + *
>>> + * @param bonding_port_id
>>> + *   Port ID of bonding device.
>>> + * @param notify_member
>>> + *   Flag of whether bonding port notifies member ports.
>>> + *
>>> + * @return
>>> + *   0 on success, negative value otherwise.
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_eth_bond_notify_member_flag_get(uint16_t bonding_port_id, bool
>>> +*notify_member);
>>> +
>>> +/**
>>> + * Notify the member ports of bonding port's information.
>>> + *
>>> + * This interface is called in the following functions:
>>> + * - bond_ethdev_lsc_event_callback()
>>> + * - bond_ethdev_configure()
>> Is this interface used just  in these cases?
>> If so I don't think it should be a API in eth_dev_op.
> Sorry I'm a bit confused.
> Do you mean "rte_eth_bond_notify_members" this interface? This interface is called in 3 functions.
User can also call this API, right?
How should the user use it? Is it when update bonding port information?
I feel like to know what the time of the notice is.
>
>>> ...
>>> +	struct rte_eth_dev *member_dev[RTE_MAX_ETHPORTS];
>>> +
>>> +	if (valid_bonding_port_id(bonding_port_id) != 0)
>>> +		return -EINVAL;
>>> +
>>> +	bond_dev = &rte_eth_devices[bonding_port_id];
>>> +	internals = bond_dev->data->dev_private;
>>> +
>>> +	for (i = 0; i < internals->member_count; i++) {
>>> +		member_port_id = internals->members[i].port_id;
>>> +		member_dev[i] = &rte_eth_devices[member_port_id];
>>> +		if (*member_dev[i]->dev_ops->bond_notify_member == NULL)
>>> +			return -ENOTSUP;
>>> +	}
>> Do we need all member ports support bond_notify_member api?
>> If allow user to select one member port to notify? This might be more general.
>> In addition, this action here is inconsistent with above handle(do not
>> notify member if doesn't supportbond_notify_member API).
> Yes, we do not need all member ports support this API.
> I just want to have a stricter restriction on this feature before, but I think your suggestion is better.
anyway, we need to have a clearly comments about this.
>
>>> ...
  

> 在 2023/10/9 11:11, Long Wu 写道:
> >> Hi Chaoyong,
> >>
> >> some comments as below.
> >>
> >>
> >> 在 2023/10/8 9:50, Chaoyong He 写道:
> >>> From: Long Wu <long.wu@corigine.com>
> >>>
> >>> Bonding PMD does not let member ports know the bonding port's
> >>> information, like how many member ports the bonding port has, what
> >>> mode the bonding port is in and so on.
> >>>
> >>> Add the notification interface for bonding port to let member port
> >>> know it is added to a bonding port and what the bonding port's
> >>> configuration is. If so the member ports have chance to achieve its
> >>> bond-flow-offlod or other private bonding functions.
> >> "its bond-flow-offlod or other private bonding functions"
> >> I wonder that what PMDs can do with this.
> >> Can you give an example in PMD to help others review?
> > After this patch series, I will send out nfp PMD code about "its bond-flow-
> offlod or other private bonding functions ".
> > I can explain here:
> > "bond-flow" means the flow rule's destination port is a bonding port.
> > Now DPDK can use bonding port as the source port in a flow rule and
> member ports can offload this flow.
> > But member ports cannot offload a flow that its destination port is a
> bonding port.
> > Because the member ports don't know the bonding port. (Of course, some
> PMDs has its self-function to let member ports know the bonding port but it
> doesn't a general "DPDK" way).
> > After this "notify patch", DPDK can do "bond-flow-offload", also "other
> private bonding functions"(like hardware balance policy, primary port
> changing and so on) can work.
> I think what you said is more like "bonding offload", right?
> It seems that you cannot do it just based on current these API in this series.
> You probably have other works to upstream for like "bond-flow" and "other
> private bonding functions".
> 

All the efforts are to make the NIC hardware aware that port has been added to the bonding port and then create a bonding port on hardware.
Based on this(Bonding port notification and member port hardware create bonding port), we can do "bond-flow-offload" or "other private bonding functions".

Taking "bond-flow-offload" as an example:
Step1: bonding port configure process --> bonding port notify member ports --> member ports know bonding port configuration --> member ports hardware creates bonding port.

Step2: app adds a flow rule about "bond-flow"(See the explanation above) --> PMD analyzes "bond-flow" then the hardware offloads this flow. (In the next patch series, and not affair the bonding PMD)

> >
> >>> Signed-off-by: Long Wu <long.wu@corigine.com>
> >>> Reviewed-by: James Hershaw <james.hershaw@corigine.com>
> >>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> >>> ---
> >>>    drivers/net/bonding/eth_bond_private.h |  1 +
> >>>    drivers/net/bonding/rte_eth_bond.h     | 46 ++++++++++++++++
> >>>    drivers/net/bonding/rte_eth_bond_api.c | 73
> >> ++++++++++++++++++++++++++
> >>>    drivers/net/bonding/rte_eth_bond_pmd.c | 27 ++++++++--
> >>>    drivers/net/bonding/version.map        |  3 ++
> >>>    lib/ethdev/ethdev_driver.h             | 18 +++++++
> >>>    6 files changed, 165 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/bonding/eth_bond_private.h
> >>> b/drivers/net/bonding/eth_bond_private.h
> >>> index e688894210..f69e85c199 100644
> >>> --- a/drivers/net/bonding/eth_bond_private.h
> >>> +++ b/drivers/net/bonding/eth_bond_private.h
> >>> @@ -180,6 +180,7 @@ struct bond_dev_private {
> >>>     uint8_t member_update_idx;
> >>>
> >>>     bool kvargs_processing_is_done;
> >>> +   bool notify_member; /**< Enable member notification of bonding port.
> >>> +*/
> >>>
> >>>     uint32_t candidate_max_rx_pktlen;
> >>>     uint32_t max_rx_pktlen;
> >>> diff --git a/drivers/net/bonding/rte_eth_bond.h
> >>> b/drivers/net/bonding/rte_eth_bond.h
> >>> index f10165f2c6..737beca446 100644
> >>> --- a/drivers/net/bonding/rte_eth_bond.h
> >>> +++ b/drivers/net/bonding/rte_eth_bond.h
> >>> @@ -351,6 +351,52 @@
> rte_eth_bond_link_up_prop_delay_set(uint16_t
> >> bonding_port_id,
> >>>    int
> >>>    rte_eth_bond_link_up_prop_delay_get(uint16_t bonding_port_id);
> >>>
> >>> +/**
> >>> + * Set the flag of whether bonding port notifies member ports.
> >>> + *
> >>> + * @param bonding_port_id
> >>> + *   Port ID of bonding device.
> >>> + * @param notify_member
> >>> + *   Flag of whether bonding port notifies member ports.
> >>> + *
> >>> + * @return
> >>> + *   0 on success, negative value otherwise.
> >>> + */
> >>> +__rte_experimental
> >>> +int
> >>> +rte_eth_bond_notify_member_flag_set(uint16_t bonding_port_id, bool
> >>> +notify_member);
> >> s/notify_membe/notify in input?
> >> because function name reveals the meaning already.
> > Thank you, you are right.
> >
> >>> +
> >>> +/**
> >>> + * Get the flag of whether bonding port notifies member ports.
> >>> + *
> >>> + * @param bonding_port_id
> >>> + *   Port ID of bonding device.
> >>> + * @param notify_member
> >>> + *   Flag of whether bonding port notifies member ports.
> >>> + *
> >>> + * @return
> >>> + *   0 on success, negative value otherwise.
> >>> + */
> >>> +__rte_experimental
> >>> +int
> >>> +rte_eth_bond_notify_member_flag_get(uint16_t bonding_port_id, bool
> >>> +*notify_member);
> >>> +
> >>> +/**
> >>> + * Notify the member ports of bonding port's information.
> >>> + *
> >>> + * This interface is called in the following functions:
> >>> + * - bond_ethdev_lsc_event_callback()
> >>> + * - bond_ethdev_configure()
> >> Is this interface used just  in these cases?
> >> If so I don't think it should be a API in eth_dev_op.
> > Sorry I'm a bit confused.
> > Do you mean "rte_eth_bond_notify_members" this interface? This interface
> is called in 3 functions.
> User can also call this API, right?

Sorry, I think the user should not call API 'rte_eth_bond_notify_members()', it's the bonding PMD call it.
There exist a so-called `notify_member_flag` in the bonding PMD, which control if the 'rte_eth_bond_notify_members()' will be invoked.
And this `notify_member_flag` is controlled by ` rte_eth_bond_notify_member_flag_set()` and ` rte_eth_bond_notify_member_flag_get()`.

> How should the user use it? Is it when update bonding port information?
> I feel like to know what the time of the notice is.
> >

My commit "net/bonding: add commands for bonding port notification " in this patch series is for app to set the "notify flag".
For example:
testpmd> port stop all
testpmd> create bonded device 4 1
testpmd> set bonding notify_member 2 enable.
testpmd> set bonding mode 4 2
testpmd> set bonding balance_xmit_policy 2 l34.
testpmd> add bonding member 0 2
testpmd> add bonding member 1 2
testpmd> port start 2
("set bonding notify_member 2 enable " is a new command to set  the "notify_member_flag".)

I also add this setting in "vdev", like:
--vdev 'net_bonding0,member=<port 1>,member=<port 2>,socket_id=1,xmit_policy=l34,notify_member=enable'

> >>> ...
> >>> +   struct rte_eth_dev *member_dev[RTE_MAX_ETHPORTS];
> >>> +
> >>> +   if (valid_bonding_port_id(bonding_port_id) != 0)
> >>> +           return -EINVAL;
> >>> +
> >>> +   bond_dev = &rte_eth_devices[bonding_port_id];
> >>> +   internals = bond_dev->data->dev_private;
> >>> +
> >>> +   for (i = 0; i < internals->member_count; i++) {
> >>> +           member_port_id = internals->members[i].port_id;
> >>> +           member_dev[i] = &rte_eth_devices[member_port_id];
> >>> +           if (*member_dev[i]->dev_ops->bond_notify_member == NULL)
> >>> +                   return -ENOTSUP;
> >>> +   }
> >> Do we need all member ports support bond_notify_member api?
> >> If allow user to select one member port to notify? This might be more
> general.
> >> In addition, this action here is inconsistent with above handle(do
> >> not notify member if doesn't supportbond_notify_member API).
> > Yes, we do not need all member ports support this API.
> > I just want to have a stricter restriction on this feature before, but I think
> your suggestion is better.
> anyway, we need to have a clearly comments about this.

Agree, I will add comments in next version.
Thanks

> >
> >>> ...