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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Tue, Feb 14, 2023 at 02:31:06PM +0200, Oz Shlomo wrote:
> 
> 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@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.
> > 
> The current act_cookie uniquely identifies the action instance.
> 

While miss_cookie also uniquely identifies the action, within a filter list.
Or, miss_cookie uniquely identifies an action configuration.

> I think it might be used in other use cases and not just for stats.

I don't disagree.

> 
> Actually, I think the current naming scheme of act_cookie and miss_cookie
> makes sense.

act_cookie is actually already used (and differently) in tc_action itself here:
1045ba77a596 ("net sched actions: Add support for user cookies")

With this, IMO it is tough to know which one takes precedence where.

Then perhaps,
act_cookie here -> instance_cookie
miss_cookie -> config_cookie

Sorry for the bikeshedding, btw, but these cookies are getting
confusing. We need them to taste nice :-}