Re: [dpdk-dev] net/failsafe: add default Tx mbuf fast free capability

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 12/21/2018 1:16 PM, Gaëtan Rivet wrote:
> Hi Ferruh, Andrew,
> 
> On Fri, Dec 21, 2018 at 03:52:07PM +0300, Andrew Rybchenko wrote:
>> Hi Ferruh,
>>
>> On 12/21/18 3:43 PM, Ferruh Yigit wrote:
>>> On 12/21/2018 12:28 PM, Andrew Rybchenko wrote:
>>>> On 12/21/18 3:12 PM, Ferruh Yigit wrote:
>>>>> On 10/12/2018 12:36 PM, Andrew Rybchenko wrote:
>>>>>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>>>
>>>>>> This capability is reported when supported by the current emitting
>>>>>> sub-device. Failsafe PMD itself does not excercise fast free logic.
>>>>> I think overlay device capability reporting already discussed a few times, the
>>>>> question is if an overlay devices should claim a feature when it depends on
>>>>> underlay devices?
>>>> The capability may be reported by the failsafe since it is transparent from
>>>> fast free logic point of view.
>>> Why it is transparent? If one of the underlying device doesn't support/claim
>>> this feature, application can't use this feature with failsafe, isn't it?
> 
> If a VF is forbidden by its PF from adding MAC addresses, the driver
> should still advertize support for "Unicast MAC filter" right?
> 
> This is the same here. Fail-safe needs to forward configurations items
> to its sub-device for a feature to work. Sometimes, the hardware won't
> be sufficient. But the fail-safe itself still has the parts needed (even
> if it is only a flag to add to a feature list).

I see your point, also I think it may be misleading for an overlay device to
announce a feature which is completely depends underlay devices. Anyway this may
be a nuance.

I am OK with the change after Andrew's explanation, and as far as I understand
you are OK too, may I add your explicit ack to the patch?

> 
> It is necessary, at the fail-safe level, to be able to describe the
> current feature set. This is what the feature list is for. There are
> important caveats to consider however, which is inherent to using the
> fail-safe.
> 
> It does not mean those features should be removed from the fail-safe
> feature list.
> 
>>
>> tx_offload_capa in failsafe is a mask to apply on sub-device capabilities.
>> So, if the capability is not supported by any sub-device it will not be
>> reported.
>> As well if there is the capability bit in the mask, it will not be reported
>> regardless
>> sub-devices capabilities. The description for the patch above tries to
>> explain it -
>> it looks like not that successful.
>>
>>>>> Given that no ack/review given to the patch, I am updating it as rejected.
>>>> Is it a new policy? I thought that it was vice versa before.
>>> Hi Andrew,
>>>
>>> Yes policy is other-way around indeed, when there is no comment at all default
>>> behavior is accept, but please take above paragraph as my comment to the patch.
>>
>> Got it.
>>
>>> And I was thinking it is a little controversial and there is no support to have
>>> it, so lets don't get it. What do you think?
>>
>> I see you motivation.
>>
>>>>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>> ---
>>>>>>    doc/guides/nics/features/failsafe.ini | 1 +
>>>>>>    drivers/net/failsafe/failsafe_ops.c   | 1 +
>>>>>>    2 files changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/doc/guides/nics/features/failsafe.ini b/doc/guides/nics/features/failsafe.ini
>>>>>> index e3c4c08f2..b6f3dcee6 100644
>>>>>> --- a/doc/guides/nics/features/failsafe.ini
>>>>>> +++ b/doc/guides/nics/features/failsafe.ini
>>>>>> @@ -7,6 +7,7 @@
>>>>>>    Link status          = Y
>>>>>>    Link status event    = Y
>>>>>>    Rx interrupt         = Y
>>>>>> +Fast mbuf free       = Y
>>>>>>    Queue start/stop     = Y
>>>>>>    Runtime Rx queue setup = Y
>>>>>>    Runtime Tx queue setup = Y
>>>>>> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
>>>>>> index 7f8bcd4c6..e3add404b 100644
>>>>>> --- a/drivers/net/failsafe/failsafe_ops.c
>>>>>> +++ b/drivers/net/failsafe/failsafe_ops.c
>>>>>> @@ -78,6 +78,7 @@ static struct rte_eth_dev_info default_infos = {
>>>>>>    		DEV_RX_OFFLOAD_SECURITY,
>>>>>>    	.tx_offload_capa =
>>>>>>    		DEV_TX_OFFLOAD_MULTI_SEGS |
>>>>>> +		DEV_TX_OFFLOAD_MBUF_FAST_FREE |
>>>>>>    		DEV_TX_OFFLOAD_IPV4_CKSUM |
>>>>>>    		DEV_TX_OFFLOAD_UDP_CKSUM |
>>>>>>    		DEV_TX_OFFLOAD_TCP_CKSUM |
>>>>>>
>>
>