Re: [PATCH net-next v9 1/7] net/sched: cls_api: Support hardware miss to tc action

[Paul Blakey <paulb@nvidia.com>]

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@nvidia.com>
>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
>>>> Acked-by: Jamal Hadi Salim <jhs@mojatatu.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.