Re: [PATCH v3 net-next] net/sched: add skbprio scheduler

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

On Fri, Jul 13, 2018 at 11:26:28AM -0700, Cong Wang wrote:
> On Fri, Jul 13, 2018 at 6:04 AM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> >
> > On Thu, Jul 12, 2018 at 11:05:45PM -0700, Cong Wang wrote:
> > > On Wed, Jul 11, 2018 at 12:33 PM Marcelo Ricardo Leitner
> > > <marcelo.leitner@gmail.com> wrote:
> > > >
> > > > On Tue, Jul 10, 2018 at 07:25:53PM -0700, Cong Wang wrote:
> > > > > On Mon, Jul 9, 2018 at 2:40 PM Marcelo Ricardo Leitner
> > > > > <marcelo.leitner@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Jul 09, 2018 at 05:03:31PM -0400, Michel Machado wrote:
> > > > > > >    Changing TC_PRIO_MAX from 15 to 63 risks breaking backward compatibility
> > > > > > > with applications.
> > > > > >
> > > > > > If done, it needs to be done carefully, indeed. I don't know if it's
> > > > > > doable, neither I know how hard is your requirement for 64 different
> > > > > > priorities.
> > > > >
> > > > > struct tc_prio_qopt {
> > > > >         int     bands;                  /* Number of bands */
> > > > >         __u8    priomap[TC_PRIO_MAX+1]; /* Map: logical priority -> PRIO band */
> > > > > };
> > > > >
> > > > > How would you do it carefully?
> > > >
> > > > quick shot, multiplex v1 and v2 formats based on bands and sizeof():
> > > >
> > > > #define TCQ_PRIO_BANDS_V1       16
> > > > #define TCQ_PRIO_BANDS_V2       64
> > > > #define TC_PRIO_MAX_V2          64
> > > >
> > > > struct tc_prio_qopt_v2 {
> > > >         int     bands;                  /* Number of bands */
> > > >         __u8    priomap[TC_PRIO_MAX_V2+1]; /* Map: logical priority -> PRIO band */
> > > > };
> > > >
> > >
> > > Good try, but:
> > >
> > > 1. You don't take padding into account, although the difference
> > > between 16 and 64 is big here. If it were 16 and 20, almost certainly
> > > wouldn't work.
> >
> > It still would work, no matter how much padding you have, as currently
> > you can't use more than 3 bands.
> 
> I am lost.
> 
> With your proposal above, you have 16 bands for V1 and 64 bands
> for V2, where does 3 come from???

My bad. s/3/16/

> 
> 
> >
> > >
> > > 2. What if I compile a new iproute2 on an old kernel? The iproute2
> > > will use V2, while old kernel has no knowledge of V2, so it only
> > > copies a part of V2 in the end....
> >
> > Yes, and that's not a problem:
> > - Either bands is > 3 and it will return EINVAL, protecting from
> >   reading beyond the buffer.
> > - Or 2 <= bands <= 3 and it will handle it as a _v1 struct, and use
> >   only the original size.
> 
> Again why 3 not 16 or 64 ??

Again, s/3/16/

> 
> Also, why does an old kernel has the logic in its binary to determine
> this?

It won't, and it doesn't need to. If you use bands > 16 with an old
kernel, it will reject per current code (that I already pasted):

        if (qopt->bands > TCQ_PRIO_BANDS || qopt->bands < 2)
                return -EINVAL;

Simple as that. If you try to use more bands than it supports, it will
reject it.

> 
> >
> > iproute2 (or other app) may still use _v1 if it wants, btw.
> 
> Yes, old iproute2 must still have v1, what's point? Are you

??

> suggesting new iproute2 should still have v1 after you propose
> v1 and v2 for kernel?

I'm only saying that both versions will be accepted by a new kernel.

> 
> I must seriously miss something. Please help.
> 
> Thanks!