Re: [dpdk-dev] [PATCH 2/2] net/bonding: support configuring Tx offloading for bonding

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 
> 
> On 2021/6/8 17:49, Andrew Rybchenko wrote:
> > "for bonding" is redundant in the summary since it is already
> > "net/bonding"
> >
> > On 4/23/21 12:46 PM, Chengchang Tang wrote:
> >> Currently, the TX offloading of the bonding device will not take effect by
> >
> > TX -> Tx
> >
> >> using dev_configure. Because the related configuration will not be
> >> delivered to the slave devices in this way.
> >
> > I think it is a major problem that Tx offloads are actually
> > ignored. It should be a patches with "Fixes:" which addresses
> > it.
> >
> >> The Tx offloading capability of the bonding device is the intersection of
> >> the capability of all slave devices. Based on this, the following functions
> >> are added to the bonding driver:
> >> 1. If a Tx offloading is within the capability of the bonding device (i.e.
> >> all the slave devices support this Tx offloading), the enabling status of
> >> the offloading of all slave devices depends on the configuration of the
> >> bonding device.
> >>
> >> 2. For the Tx offloading that is not within the Tx offloading capability
> >> of the bonding device, the enabling status of the offloading on the slave
> >> devices is irrelevant to the bonding device configuration. And it depends
> >> on the original configuration of the slave devices.
> >>
> >> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >> ---
> >>  drivers/net/bonding/rte_eth_bond_pmd.c | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> >> index 84af348..9922657 100644
> >> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> >> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> >> @@ -1712,6 +1712,8 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> >>  	struct rte_flow_error flow_error;
> >>
> >>  	struct bond_dev_private *internals = bonded_eth_dev->data->dev_private;
> >> +	uint64_t tx_offload_cap = internals->tx_offload_capa;
> >> +	uint64_t tx_offload;
> >>
> >>  	/* Stop slave */
> >>  	errval = rte_eth_dev_stop(slave_eth_dev->data->port_id);
> >> @@ -1759,6 +1761,17 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> >>  		slave_eth_dev->data->dev_conf.rxmode.offloads &=
> >>  				~DEV_RX_OFFLOAD_JUMBO_FRAME;
> >>
> >> +	while (tx_offload_cap != 0) {
> >> +		tx_offload = 1ULL << __builtin_ctzll(tx_offload_cap);
> >> +		if (bonded_eth_dev->data->dev_conf.txmode.offloads & tx_offload)
> >> +			slave_eth_dev->data->dev_conf.txmode.offloads |=
> >> +				tx_offload;
> >> +		else
> >> +			slave_eth_dev->data->dev_conf.txmode.offloads &=
> >> +				~tx_offload;
> >> +		tx_offload_cap &= ~tx_offload;
> >> +	}
> >> +
> >
> > Frankly speaking I don't understand why it is that complicated.
> > ethdev rejects of unsupported Tx offloads. So, can't we simply:
> > slave_eth_dev->data->dev_conf.txmode.offloads =
> >     bonded_eth_dev->data->dev_conf.txmode.offloads;
> >
> 
> Using such a complicated method is to increase the flexibility of the slave devices,
> allowing the Tx offloading of the slave devices to be incompletely consistent with
> the bond device. If some offloading can be turned on without bond device awareness,
> they can be retained in this case.


Not sure how that can that happen...
From my understanding tx_offload for bond device has to be intersection of tx_offloads
of all slaves, no? Otherwise bond device might be misconfigured.
Anyway for that code snippet above, wouldn't the same be achived by:
slave_eth_dev->data->dev_conf.txmode.offloads &= internals->tx_offload_capa & bonded_eth_dev->data->dev_conf.txmode.offloads;
?