diff mbox series

[RFC,v2,net-next,03/28] net/sched: act_api: increase TCA_ID_MAX

Message ID 20230517110232.29349-3-jhs@mojatatu.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series Introducing P4TC | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches (and no cover letter)
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 4196 this patch: 4196
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 921 this patch: 921
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4415 this patch: 4415
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 11 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jamal Hadi Salim May 17, 2023, 11:02 a.m. UTC 
Increase TCA_ID_MAX from 255 to 1023

Given P4TC dynamic actions required new IDs (dynamically) and 30 of those are
already taken by the standard actions (such as gact, mirred and ife) we are left
with 225 actions to create, which seems like a small number.

Signed-off-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Suggested-by: Vlad Buslov <vladbu@nvidia.com>
---
 include/uapi/linux/pkt_cls.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Marcelo Ricardo Leitner June 2, 2023, 2:19 p.m. UTC | #1 
On Wed, May 17, 2023 at 07:02:07AM -0400, Jamal Hadi Salim wrote:
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -140,9 +140,9 @@ enum tca_id {
>  	TCA_ID_MPLS,
>  	TCA_ID_CT,
>  	TCA_ID_GATE,
> -	TCA_ID_DYN,
> +	TCA_ID_DYN = 256,
>  	/* other actions go here */
> -	__TCA_ID_MAX = 255
> +	__TCA_ID_MAX = 1023
>  };

It feels that this patch should go together with the 1st one, when
dynamic actions were introduced. When I was reading that patch, I was
wondering about possible conflicts with a new loaded dynamic action
and some userspace app using a new tca_id definition (it is UAPI,
after all) that overlaps with it.

  Marcelo
Jamal Hadi Salim June 3, 2023, 1:03 p.m. UTC | #2 
On Fri, Jun 2, 2023 at 10:19 AM Marcelo Ricardo Leitner
<mleitner@redhat.com> wrote:
>
> On Wed, May 17, 2023 at 07:02:07AM -0400, Jamal Hadi Salim wrote:
> > --- a/include/uapi/linux/pkt_cls.h
> > +++ b/include/uapi/linux/pkt_cls.h
> > @@ -140,9 +140,9 @@ enum tca_id {
> >       TCA_ID_MPLS,
> >       TCA_ID_CT,
> >       TCA_ID_GATE,
> > -     TCA_ID_DYN,
> > +     TCA_ID_DYN = 256,
> >       /* other actions go here */
> > -     __TCA_ID_MAX = 255
> > +     __TCA_ID_MAX = 1023
> >  };
>
> It feels that this patch should go together with the 1st one, when
> dynamic actions were introduced. When I was reading that patch, I was
> wondering about possible conflicts with a new loaded dynamic action
> and some userspace app using a new tca_id definition (it is UAPI,
> after all) that overlaps with it.

Good catch. We'll make this change in the next update.

cheers,
jamal
Jakub Kicinski June 5, 2023, 5:39 p.m. UTC | #3 
On Wed, 17 May 2023 07:02:07 -0400 Jamal Hadi Salim wrote:
> Increase TCA_ID_MAX from 255 to 1023
> 
> Given P4TC dynamic actions required new IDs (dynamically) and 30 of those are
> already taken by the standard actions (such as gact, mirred and ife) we are left
> with 225 actions to create, which seems like a small number.

> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 5b66df3ec332..337411949ad0 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -140,9 +140,9 @@ enum tca_id {
>  	TCA_ID_MPLS,
>  	TCA_ID_CT,
>  	TCA_ID_GATE,
> -	TCA_ID_DYN,
> +	TCA_ID_DYN = 256,
>  	/* other actions go here */
> -	__TCA_ID_MAX = 255
> +	__TCA_ID_MAX = 1023
>  };
>  
>  #define TCA_ID_MAX __TCA_ID_MAX

I haven't look at any of the patches but this stands out as bad idea
on the surface.
Jamal Hadi Salim June 6, 2023, 5:04 p.m. UTC | #4 
On Mon, Jun 5, 2023 at 5:19 PM Jakub Kicinski via p4tc-discussions
<p4tc-discussions@netdevconf.info> wrote:
>
> On Wed, 17 May 2023 07:02:07 -0400 Jamal Hadi Salim wrote:
> > Increase TCA_ID_MAX from 255 to 1023
> >
> > Given P4TC dynamic actions required new IDs (dynamically) and 30 of those are
> > already taken by the standard actions (such as gact, mirred and ife) we are left
> > with 225 actions to create, which seems like a small number.
>
> > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> > index 5b66df3ec332..337411949ad0 100644
> > --- a/include/uapi/linux/pkt_cls.h
> > +++ b/include/uapi/linux/pkt_cls.h
> > @@ -140,9 +140,9 @@ enum tca_id {
> >       TCA_ID_MPLS,
> >       TCA_ID_CT,
> >       TCA_ID_GATE,
> > -     TCA_ID_DYN,
> > +     TCA_ID_DYN = 256,
> >       /* other actions go here */
> > -     __TCA_ID_MAX = 255
> > +     __TCA_ID_MAX = 1023
> >  };
> >
> >  #define TCA_ID_MAX __TCA_ID_MAX
>
> I haven't look at any of the patches but this stands out as bad idea
> on the surface.

The idea is to reserve a range of the IDs for dynamic use in this case
from 256-1023. The kernel will issue an action id from that range when
we request it. The assumption is someone adding a "static" action ID
will populate the above enum and is able to move the range boundaries.
P4TC continues to work with old and new kernels and with old and new
tc.
Did i miss something you were alluding to?

cheers,
jamal
Jakub Kicinski June 6, 2023, 5:15 p.m. UTC | #5 
On Tue, 6 Jun 2023 13:04:18 -0400 Jamal Hadi Salim wrote:
> > > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> > > index 5b66df3ec332..337411949ad0 100644
> > > --- a/include/uapi/linux/pkt_cls.h
> > > +++ b/include/uapi/linux/pkt_cls.h
> > > @@ -140,9 +140,9 @@ enum tca_id {
> > >       TCA_ID_MPLS,
> > >       TCA_ID_CT,
> > >       TCA_ID_GATE,
> > > -     TCA_ID_DYN,
> > > +     TCA_ID_DYN = 256,
> > >       /* other actions go here */
> > > -     __TCA_ID_MAX = 255
> > > +     __TCA_ID_MAX = 1023
> > >  };
> > >
> > >  #define TCA_ID_MAX __TCA_ID_MAX  
> >
> > I haven't look at any of the patches but this stands out as bad idea
> > on the surface.  
> 
> The idea is to reserve a range of the IDs for dynamic use in this case
> from 256-1023. The kernel will issue an action id from that range when
> we request it. The assumption is someone adding a "static" action ID
> will populate the above enum and is able to move the range boundaries.
> P4TC continues to work with old and new kernels and with old and new
> tc.
> Did i miss something you were alluding to?

Allocating action IDs for P4 at the same level as normal TC actions
makes the P4 stuff looks like a total parallel implementation to me.
Why is there not a TCA_ID_P4 which muxes internally?
AFAIU interpretation of action attributes depends on the ID, which
means that user space to parse the action attrs has to not only look 
at the ID but now also resolve what that ID means.
Jamal Hadi Salim June 6, 2023, 6:52 p.m. UTC | #6 
On Tue, Jun 6, 2023 at 1:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 6 Jun 2023 13:04:18 -0400 Jamal Hadi Salim wrote:
> > > > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> > > > index 5b66df3ec332..337411949ad0 100644
> > > > --- a/include/uapi/linux/pkt_cls.h
> > > > +++ b/include/uapi/linux/pkt_cls.h
> > > > @@ -140,9 +140,9 @@ enum tca_id {
> > > >       TCA_ID_MPLS,
> > > >       TCA_ID_CT,
> > > >       TCA_ID_GATE,
> > > > -     TCA_ID_DYN,
> > > > +     TCA_ID_DYN = 256,
> > > >       /* other actions go here */
> > > > -     __TCA_ID_MAX = 255
> > > > +     __TCA_ID_MAX = 1023
> > > >  };
> > > >
> > > >  #define TCA_ID_MAX __TCA_ID_MAX
> > >
> > > I haven't look at any of the patches but this stands out as bad idea
> > > on the surface.
> >
> > The idea is to reserve a range of the IDs for dynamic use in this case
> > from 256-1023. The kernel will issue an action id from that range when
> > we request it. The assumption is someone adding a "static" action ID
> > will populate the above enum and is able to move the range boundaries.
> > P4TC continues to work with old and new kernels and with old and new
> > tc.
> > Did i miss something you were alluding to?
>
> Allocating action IDs for P4 at the same level as normal TC actions
> makes the P4 stuff looks like a total parallel implementation to me.

P4 actions  look exactly the same as standard tc actions (semantics,
attributes, etc) so it seemed natural to just reuse the same
mechanics. A P4 match can invoke tc police action for example if
that's what P4 program defines. The only addition we have are
separation "static" (predefined in the kernel or a kernel module)
actions vs "dynamic" set of actions.

> Why is there not a TCA_ID_P4 which muxes internally?

It seemed like an unnecessary indirection given that everything else
is the same.

> AFAIU interpretation of action attributes depends on the ID, which
> means that user space to parse the action attrs has to not only look
> at the ID but now also resolve what that ID means.

Parsing what you are saying above (and thinking while typing this):
In the classical control path "tc action ..."  or when you say "tc
filter... action .." only names are passed. Action names are unique
globally so this works.  So no resolution required there.

P4 spec says an action is allowed to call another action "static" or
"dynamic" - and we can tell the dynamic action to find it by name or
optimally by ID (hence this lookup_byid). Static actions are easy,
they are in the UAPI because their IDs are compiled in. Dynamic
actions optimization requires resolution as you mentioned. Your
suggestion may work then we wont need to define the ranges at all (and
maintain the status quo of passing names instead of IDs from user
space).
Let me socialize it with the rest of the group...

cheers,
jamal
diff mbox series

Patch

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 5b66df3ec332..337411949ad0 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -140,9 +140,9 @@  enum tca_id {
 	TCA_ID_MPLS,
 	TCA_ID_CT,
 	TCA_ID_GATE,
-	TCA_ID_DYN,
+	TCA_ID_DYN = 256,
 	/* other actions go here */
-	__TCA_ID_MAX = 255
+	__TCA_ID_MAX = 1023
 };
 
 #define TCA_ID_MAX __TCA_ID_MAX