[2/2] net/bonding: support configuring Tx offloading for bonding
Checks
Commit Message
Currently, the TX offloading of the bonding device will not take effect by
using dev_configure. Because the related configuration will not be
delivered to the slave devices in this way.
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(+)
--
2.7.4
Comments
"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;
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.
>
> .
>
>
>
> 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;
?
On 6/9/21 12:11 PM, Ananyev, Konstantin wrote:
>
>>
>>
>> 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...
+1
@Chengchang could you provide an example how it could 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;
> ?
Hi, Andrew and Ananyev
On 2021/6/9 17:37, Andrew Rybchenko wrote:
> On 6/9/21 12:11 PM, Ananyev, Konstantin wrote:
>>
>>>
>>>
>>> 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...
>
> +1
>
> @Chengchang could you provide an example how it could happen.
>
For example:
device 1 capability: VLAN_INSERT | MBUF_FAST_FREE
device 2 capability: VLAN_INSERT
And the capability of bonded device will be VLAN_INSERT.
So, we can only set VLAN_INSERT for the bonded device. So what if we want to enable
MBUF_FAST_FREE in device 1 to improve performance? For the application, as long as it
can guarantee the condition of MBUF ref_cnt = 1, then it can run normally if
MBUF_FAST_FREE is turned on.
In my logic, if device 1 has been configured with MBUF_FAST_FREE, and then
added to the bonded device as a slave. The MBUF_FAST_FREE will be reserved.
>> 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;
>> ?
>
I think it will not achieved my purpose in the scenario I mentioned above.
> .
>
> Hi, Andrew and Ananyev
>
> On 2021/6/9 17:37, Andrew Rybchenko wrote:
> > On 6/9/21 12:11 PM, Ananyev, Konstantin wrote:
> >>
> >>>
> >>>
> >>> 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...
> >
> > +1
> >
> > @Chengchang could you provide an example how it could happen.
> >
>
> For example:
> device 1 capability: VLAN_INSERT | MBUF_FAST_FREE
> device 2 capability: VLAN_INSERT
> And the capability of bonded device will be VLAN_INSERT.
> So, we can only set VLAN_INSERT for the bonded device. So what if we want to enable
> MBUF_FAST_FREE in device 1 to improve performance? For the application, as long as it
> can guarantee the condition of MBUF ref_cnt = 1, then it can run normally if
> MBUF_FAST_FREE is turned on.
>
> In my logic, if device 1 has been configured with MBUF_FAST_FREE, and then
> added to the bonded device as a slave. The MBUF_FAST_FREE will be reserved.
So your intention is to allow slave device silently overrule master tx_offload settings?
If so, I don't think it is a good idea - sounds like potentially bogus and error prone approach.
Second thing - I still don't see how the code above can help you with it.
From what I read in your code - you clear tx_offload bits that are not not supported by the master.
>
> >> 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;
> >> ?
> >
>
> I think it will not achieved my purpose in the scenario I mentioned above.
>
> > .
> >
On 6/14/21 2:05 PM, Ananyev, Konstantin wrote:
>
>
>> Hi, Andrew and Ananyev
>>
>> On 2021/6/9 17:37, Andrew Rybchenko wrote:
>>> On 6/9/21 12:11 PM, Ananyev, Konstantin wrote:
>>>>
>>>>>
>>>>>
>>>>> 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...
>>>
>>> +1
>>>
>>> @Chengchang could you provide an example how it could happen.
>>>
>>
>> For example:
>> device 1 capability: VLAN_INSERT | MBUF_FAST_FREE
>> device 2 capability: VLAN_INSERT
>> And the capability of bonded device will be VLAN_INSERT.
>> So, we can only set VLAN_INSERT for the bonded device. So what if we want to enable
>> MBUF_FAST_FREE in device 1 to improve performance? For the application, as long as it
>> can guarantee the condition of MBUF ref_cnt = 1, then it can run normally if
>> MBUF_FAST_FREE is turned on.
>>
>> In my logic, if device 1 has been configured with MBUF_FAST_FREE, and then
>> added to the bonded device as a slave. The MBUF_FAST_FREE will be reserved.
>
> So your intention is to allow slave device silently overrule master tx_offload settings?
> If so, I don't think it is a good idea - sounds like potentially bogus and error prone approach.
+1
> Second thing - I still don't see how the code above can help you with it.
> From what I read in your code - you clear tx_offload bits that are not not supported by the master.
+1
>>
>>>> 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;
>>>> ?
>>>
>>
>> I think it will not achieved my purpose in the scenario I mentioned above.
>>
>>> .
>>>
>
@@ -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;
+ }
+
nb_rx_queues = bonded_eth_dev->data->nb_rx_queues;
nb_tx_queues = bonded_eth_dev->data->nb_tx_queues;