From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Subject: Re: [PATCH v3 net-next] net/sched: add skbprio scheduler Date: Sat, 14 Jul 2018 01:39:19 -0300 Message-ID: <20180714043917.GA20383@localhost.localdomain> References: <20180709154409.GC8880@localhost.localdomain> <20180709195319.GD8880@localhost.localdomain> <20180709214016.GD10923@localhost.localdomain> <20180711193336.GF8880@localhost.localdomain> <20180713130452.GJ8880@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Michel Machado , Nishanth Devarajan , Jamal Hadi Salim , Jiri Pirko , David Miller , Linux Kernel Network Developers , Cody Doucette To: Cong Wang Return-path: Received: from mail-pf0-f193.google.com ([209.85.192.193]:46452 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725851AbeGNE47 (ORCPT ); Sat, 14 Jul 2018 00:56:59 -0400 Received: by mail-pf0-f193.google.com with SMTP id l123-v6so23673570pfl.13 for ; Fri, 13 Jul 2018 21:39:23 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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 > 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 > > > 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 > > > > > 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!