[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8232a755-fea4-6701-badc-a684c5b22b20@nvidia.com>
Date: Tue, 14 Feb 2023 14:31:06 +0200
From: Oz Shlomo <ozsh@...dia.com>
To: Paul Blakey <paulb@...dia.com>,
Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc: netdev@...r.kernel.org, Saeed Mahameed <saeedm@...dia.com>,
Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jiri Pirko <jiri@...dia.com>, Roi Dayan <roid@...dia.com>,
Vlad Buslov <vladbu@...dia.com>,
Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH net-next v9 1/7] net/sched: cls_api: Support hardware miss
to tc action
On 14/02/2023 14:14, Paul Blakey wrote:
> On 13/02/2023 20:43, Marcelo Ricardo Leitner wrote:
>> On Mon, Feb 13, 2023 at 06:13:34PM +0200, Paul Blakey wrote:
>>> On 10/02/2023 04:21, Marcelo Ricardo Leitner wrote:
>>>> On Mon, Feb 06, 2023 at 07:43:57PM +0200, Paul Blakey wrote:
>>>>> For drivers to support partial offload of a filter's action list,
>>>>> add support for action miss to specify an action instance to
>>>>> continue from in sw.
>>>>>
>>>>> CT action in particular can't be fully offloaded, as new connections
>>>>> need to be handled in software. This imposes other limitations on
>>>>> the actions that can be offloaded together with the CT action, such
>>>>> as packet modifications.
>>>>>
>>>>> Assign each action on a filter's action list a unique miss_cookie
>>>>> which drivers can then use to fill action_miss part of the tc skb
>>>>> extension. On getting back this miss_cookie, find the action
>>>>> instance with relevant cookie and continue classifying from there.
>>>>>
>>>>> Signed-off-by: Paul Blakey <paulb@...dia.com>
>>>>> Reviewed-by: Jiri Pirko <jiri@...dia.com>
>>>>> Reviewed-by: Simon Horman <simon.horman@...igine.com>
>>>>> Acked-by: Jamal Hadi Salim <jhs@...atatu.com>
>>>>> ---
>>>>> include/linux/skbuff.h | 6 +-
>>>>> include/net/flow_offload.h | 1 +
>>>>> include/net/pkt_cls.h | 34 +++---
>>>>> include/net/sch_generic.h | 2 +
>>>>> net/openvswitch/flow.c | 3 +-
>>>>> net/sched/act_api.c | 2 +-
>>>>> net/sched/cls_api.c | 213 +++++++++++++++++++++++++++++++++++--
>>>>> 7 files changed, 234 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>>> index 1fa95b916342..9b9aa854068f 100644
>>>>> --- a/include/linux/skbuff.h
>>>>> +++ b/include/linux/skbuff.h
>>>>> @@ -311,12 +311,16 @@ struct nf_bridge_info {
>>>>> * and read by ovs to recirc_id.
>>>>> */
>>>>> struct tc_skb_ext {
>>>>> - __u32 chain;
>>>>> + union {
>>>>> + u64 act_miss_cookie;
>>>>> + __u32 chain;
>>>>> + };
>>>>> __u16 mru;
>>>>> __u16 zone;
>>>>> u8 post_ct:1;
>>>>> u8 post_ct_snat:1;
>>>>> u8 post_ct_dnat:1;
>>>>> + u8 act_miss:1; /* Set if act_miss_cookie is used */
>>>>> };
>>>>> #endif
>>>>> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>>>>> index 0400a0ac8a29..88db7346eb7a 100644
>>>>> --- a/include/net/flow_offload.h
>>>>> +++ b/include/net/flow_offload.h
>>>>> @@ -228,6 +228,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
>>>>> struct flow_action_entry {
>>>>> enum flow_action_id id;
>>>>> u32 hw_index;
>>>>> + u64 miss_cookie;
>>>> The per-action stats patchset is adding a cookie for the actions as
>>>> well, and exactly on this struct:
>>>>
>>>> @@ -228,6 +228,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data,
>>>> struct flow_action_entry {
>>>> enum flow_action_id id;
>>>> u32 hw_index;
>>>> + unsigned long act_cookie;
>>>> enum flow_action_hw_stats hw_stats;
>>>> action_destr destructor;
>>>> void *destructor_priv;
>>>>
>>>> There, it is a simple value: the act pointer itself. Here, it is already more
>>>> complex. Can them be merged into only one maybe?
>>>> If not, perhaps act_cookie should be renamed to stats_cookie then.
>>> I don't think it can be shared, actions can be shared between multiple
>>> filters, while the miss cookie would be different for each used instance
>>> (takes the filter in to account).
>> Good point. So it would at best be a masked value that part A works
>> for the miss here and part B for the stats, which is pretty much what
>> the two cookies are giving, just without having to do bit gymnasics,
>> yes.
> act cookie is using 64 bits (to store the pointer and void a mapping), and I'm using at least
>
> 32bits, so there is not simple type that will contain both.
>
> So I'll rename the act_cookie to stats_cookie once I rebase.
>
The current act_cookie uniquely identifies the action instance.
I think it might be used in other use cases and not just for stats.
Actually, I think the current naming scheme of act_cookie and
miss_cookie makes sense.
Powered by blists - more mailing lists