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

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

On Fri, Dec 21, 2018 at 01:59:20PM +0000, Ferruh Yigit wrote:
> 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?
> 

Yes the patch is ok for me, thank you Ferruh.

> > 
> > 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 |
> >>>>>>
> >>
> > 
> 

-- 
Gaëtan Rivet
6WIND