ACK/Cmnt: [SRU][F:linux-bluefield][PATCH] UBUNTU: SAUCE: netfilter: flowtable: Do flow offload refresh when requested

[Roi Dayan]

On 2021-05-07 9:30 PM, Daniel Jurgens wrote:
>> -----Original Message-----
>> From: Tim Gardner <tim.gardner at canonical.com>
>> Sent: Friday, May 7, 2021 10:12 AM
>>
>> Acked-by: Tim Gardner <tim.gardner at canonical.com>
>>
>> If I'm reading this right, this patch is dealing with offload request responses
>> from NIC firmware, right ?
>>
>> I'm not convinced this will pass an upstream review because it seems like a
>> bit a of a hack, but then the patch this is fixing 8b3646d6e0c4
>> ("net/sched: act_ct: Support refreshing the flow table entries") seems like a
>> bit of a hack too. Sometimes you just have to work around firmware
>> behaviors.
>>
>> Feel free to tell me if I'm completely wrong :)
> 
> I'll let Roi answer this one.
> 

Hi,

I started to discuss about this with Pablo, netfilter maintainer.
explaining my thoughts that this bit is redundant really.
I do hope to do get this hw refresh bit removed. as tried to explain in
the commit message, the later added hw pending bit already protects from
trying to create a new work if not needed and we do check if the
flowtable has hw offload bit anyway if tuples in this flowtable should
be offloaded. there is no point for this bit and it causing issue today
and can cause issue later if/when more conditions will stop the current
offload but wont set the refresh bit actively.
even today the single place that set the refresh bit has a race that
the hw pending bit was not removed so new skb could clear the refresh
bit and stop because of the hw pending bit still exists and no refresh
will happen ever again. ending with connections that never gets
offloaded even though possible.

Thanks,
Roi


>>
>> On 5/6/21 6:46 AM, Daniel Jurgens wrote:
>>> BugLink: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.launchpad.net%2Fbugs%2F1927374&data=04%7C01%7Croid%40nvidia.com%7Cae611c26cea24e1ca6d608d9118638a6%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637560090442320882%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=anO2XFv6KZQSBR%2BEMIiI1UUI4TgazJrzg%2FwZmxsj%2Fr8%3D&reserved=0
>>>
>>> Offload could fail for multiple reasons and a hw refresh bit is set to
>>> try to reoffload it in next sw packet.
>>> But sometimes the hw refresh bit is not set but a refresh could succeed.
>>> Remove the hw refresh bit and do offload refresh if requested.
>>> There won't be a new work entry if a work is already pending anyway as
>>> there is the hw pending bit.
>>>
>>> Fixes: 8b3646d6e0c4 ("net/sched: act_ct: Support refreshing the flow
>>> table entries")
>>> Signed-off-by: Roi Dayan <roid at nvidia.com>
>>> Signed-off-by: Daniel Jurgens <danielj at nvidia.com>
>>> ---
>>>    include/net/netfilter/nf_flow_table.h | 1 -
>>>    net/netfilter/nf_flow_table_core.c    | 3 +--
>>>    net/netfilter/nf_flow_table_offload.c | 7 ++++---
>>>    3 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/net/netfilter/nf_flow_table.h
>>> b/include/net/netfilter/nf_flow_table.h
>>> index ff14b39..ba3b58a 100644
>>> --- a/include/net/netfilter/nf_flow_table.h
>>> +++ b/include/net/netfilter/nf_flow_table.h
>>> @@ -126,7 +126,6 @@ enum nf_flow_flags {
>>>    	NF_FLOW_HW,
>>>    	NF_FLOW_HW_DYING,
>>>    	NF_FLOW_HW_DEAD,
>>> -	NF_FLOW_HW_REFRESH,
>>>    	NF_FLOW_HW_PENDING,
>>>    };
>>>
>>> diff --git a/net/netfilter/nf_flow_table_core.c
>>> b/net/netfilter/nf_flow_table_core.c
>>> index 64df3f8..2405eac 100644
>>> --- a/net/netfilter/nf_flow_table_core.c
>>> +++ b/net/netfilter/nf_flow_table_core.c
>>> @@ -261,8 +261,7 @@ void flow_offload_refresh(struct nf_flowtable
>> *flow_table,
>>>    	flow->timeout = nf_flowtable_time_stamp +
>>>    			nf_flow_offload_timeout(flow_table);
>>>
>>> -	if (likely(!nf_flowtable_hw_offload(flow_table) ||
>>> -		   !test_and_clear_bit(NF_FLOW_HW_REFRESH, &flow-
>>> flags)))
>>> +	if (likely(!nf_flowtable_hw_offload(flow_table)))
>>>    		return;
>>>
>>>    	nf_flow_offload_add(flow_table, flow); diff --git
>>> a/net/netfilter/nf_flow_table_offload.c
>>> b/net/netfilter/nf_flow_table_offload.c
>>> index 28a2983..ad705ca 100644
>>> --- a/net/netfilter/nf_flow_table_offload.c
>>> +++ b/net/netfilter/nf_flow_table_offload.c
>>> @@ -759,10 +759,11 @@ static void flow_offload_work_add(struct
>>> flow_offload_work *offload)
>>>
>>>    	err = flow_offload_rule_add(offload, flow_rule);
>>>    	if (err < 0)
>>> -		set_bit(NF_FLOW_HW_REFRESH, &offload->flow->flags);
>>> -	else
>>> -		set_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
>>> +		goto out;
>>> +
>>> +	set_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
>>>
>>> +out:
>>>    	nf_flow_offload_destroy(flow_rule);
>>>    }
>>>
>>>
>>
>> --
>> -----------
>> Tim Gardner
>> Canonical, Inc