From: Paul Blakey <paulb@mellanox.com>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Oz Shlomo <ozsh@mellanox.com>, Roi Dayan <roid@mellanox.com>,
netdev@vger.kernel.org, Saeed Mahameed <saeedm@mellanox.com>,
netfilter-devel@vger.kernel.org
Subject: Re: [PATCH net] netfilter: flowtable: Add pending bit for offload work
Date: Mon, 11 May 2020 11:32:36 +0300 [thread overview]
Message-ID: <9dff92fe-15cd-348d-ff1c-7a102ea9263c@mellanox.com> (raw)
In-Reply-To: <20200510221434.GA11226@salvia>
On 5/11/2020 1:14 AM, Pablo Neira Ayuso wrote:
> Hi,
>
> On Wed, May 06, 2020 at 02:24:39PM +0300, Paul Blakey wrote:
>> Gc step can queue offloaded flow del work or stats work.
>> Those work items can race each other and a flow could be freed
>> before the stats work is executed and querying it.
>> To avoid that, add a pending bit that if a work exists for a flow
>> don't queue another work for it.
>> This will also avoid adding multiple stats works in case stats work
>> didn't complete but gc step started again.
> This is happening since the mutex has been removed, right?
>
> Another question below.
it's from the using a new workqueue and one work per item, allowing parallelization
between a flow work items.
>
>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>> ---
>> include/net/netfilter/nf_flow_table.h | 1 +
>> net/netfilter/nf_flow_table_offload.c | 8 +++++++-
>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
>> index 6bf6965..c54a7f7 100644
>> --- a/include/net/netfilter/nf_flow_table.h
>> +++ b/include/net/netfilter/nf_flow_table.h
>> @@ -127,6 +127,7 @@ enum nf_flow_flags {
>> NF_FLOW_HW_DYING,
>> NF_FLOW_HW_DEAD,
>> NF_FLOW_HW_REFRESH,
>> + NF_FLOW_HW_PENDING,
>> };
>>
>> enum flow_offload_type {
>> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
>> index b9d5ecc..731d738 100644
>> --- a/net/netfilter/nf_flow_table_offload.c
>> +++ b/net/netfilter/nf_flow_table_offload.c
>> @@ -817,6 +817,7 @@ static void flow_offload_work_handler(struct work_struct *work)
>> WARN_ON_ONCE(1);
>> }
>>
>> + clear_bit(NF_FLOW_HW_PENDING, &offload->flow->flags);
>> kfree(offload);
>> }
>>
>> @@ -831,9 +832,14 @@ static void flow_offload_queue_work(struct flow_offload_work *offload)
>> {
>> struct flow_offload_work *offload;
>>
>> + if (test_and_set_bit(NF_FLOW_HW_PENDING, &flow->flags))
>> + return NULL;
> In case of stats, it's fine to lose work.
>
> But how does this work for the deletion case? Does this falls back to
> the timeout deletion?
We get to nf_flow_table_offload_del (delete) in these cases:
>-------if (nf_flow_has_expired(flow) || nf_ct_is_dying(flow->ct) ||
>------- test_bit(NF_FLOW_TEARDOWN, &flow->flags) {
>------->------- ....
>------->------- nf_flow_offload_del(flow_table, flow);
Which are all persistent once set but the nf_flow_has_expired(flow). So we will
try the delete
again and again till pending flag is unset or the flow is 'saved' by the already
queued stats updating the timeout.
A pending stats update can't save the flow once it's marked for teardown or
(flow->ct is dying), only delay it.
We didn't mention flush, like in table free. I guess we need to flush the
hardware workqueue
of any pending stats work, then queue the deletion, and flush again:
Adding nf_flow_table_offload_flush(flow_table), after
cancel_delayed_work_sync(&flow_table->gc_work);
>
> Thanks.
next prev parent reply other threads:[~2020-05-11 8:32 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-06 11:24 [PATCH net] netfilter: flowtable: Add pending bit for offload work Paul Blakey
2020-05-10 22:14 ` Pablo Neira Ayuso
2020-05-11 8:32 ` Paul Blakey [this message]
2020-05-11 11:59 ` Pablo Neira Ayuso
2020-05-11 13:57 ` Roi Dayan
2020-05-11 13:58 ` Roi Dayan
2020-05-11 14:37 ` Pablo Neira Ayuso
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9dff92fe-15cd-348d-ff1c-7a102ea9263c@mellanox.com \
--to=paulb@mellanox.com \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=ozsh@mellanox.com \
--cc=pablo@netfilter.org \
--cc=roid@mellanox.com \
--cc=saeedm@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).