| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGWr4cTaUcX+QWsQoDALRJEyEuD5Tm8fv2d6K4=FSYTHQrcMTA@mail.gmail.com>
Date: Tue, 1 Apr 2025 09:36:03 -0700
From: Octavian Purdila <tavip@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Cong Wang <xiyou.wangcong@...il.com>, jhs@...atatu.com,
jiri@...nulli.us, davem@...emloft.net, kuba@...nel.org, horms@...nel.org,
shuah@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net 1/3] net_sched: sch_sfq: use a temporary work area for
validating configuration
On Tue, Apr 1, 2025 at 4:13 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 4/1/25 12:47 PM, Eric Dumazet wrote:
> > On Tue, Apr 1, 2025 at 11:27 AM Paolo Abeni <pabeni@...hat.com> wrote:
> >> On 3/31/25 1:49 AM, Cong Wang wrote:
> >>> On Fri, Mar 28, 2025 at 01:16:32PM -0700, Octavian Purdila wrote:
> >>>> diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
> >>>> index 65d5b59da583..027a3fde2139 100644
> >>>> --- a/net/sched/sch_sfq.c
> >>>> +++ b/net/sched/sch_sfq.c
> >>>> @@ -631,6 +631,18 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
> >>>> struct red_parms *p = NULL;
> >>>> struct sk_buff *to_free = NULL;
> >>>> struct sk_buff *tail = NULL;
> >>>> + /* work area for validating changes before committing them */
> >>>> + struct {
> >>>> + int limit;
> >>>> + unsigned int divisor;
> >>>> + unsigned int maxflows;
> >>>> + int perturb_period;
> >>>> + unsigned int quantum;
> >>>> + u8 headdrop;
> >>>> + u8 maxdepth;
> >>>> + u8 flags;
> >>>> + } tmp;
> >>>
> >>> Thanks for your patch. It reminds me again about the lacking of complete
> >>> RCU support in TC. ;-)
> >>>
> >>> Instead of using a temporary struct, how about introducing a new one
> >>> called struct sfq_sched_opt and putting it inside struct sfq_sched_data?
> >>> It looks more elegant to me.
> >>
> >> I agree with that. It should also make the code more compact. @Octavian,
> >> please update the patch as per Cong's suggestion.
> >
> > The concern with this approach was data locality.
>
> I did not consider that aspect.
>
> How about not using the struct at all, then?
>
> int cur_limit;
> // ...
> u8 cur_flags;
>
> the 'tmp' struct is IMHO not so nice.
>
Thanks Paolo and Cong for the review.
I'll drop the structure, my initial thoughts were that by grouping
them into a structure it would make it easier to understand the
purpose of those variables, especially since we have a lot of local
variables now.
> > I had in my TODO list a patch to remove (accumulated over time) holes
> > and put together hot fields.
> >
> > Something like :
> >
> > struct sfq_sched_data {
> > int limit; /* 0 0x4 */
> > unsigned int divisor; /* 0x4 0x4 */
> > u8 headdrop; /* 0x8 0x1 */
> > u8 maxdepth; /* 0x9 0x1 */
> > u8 cur_depth; /* 0xa 0x1 */
> > u8 flags; /* 0xb 0x1 */
> > unsigned int quantum; /* 0xc 0x4 */
> > siphash_key_t perturbation; /* 0x10 0x10 */
> > struct tcf_proto * filter_list; /* 0x20 0x8 */
> > struct tcf_block * block; /* 0x28 0x8 */
> > sfq_index * ht; /* 0x30 0x8 */
> > struct sfq_slot * slots; /* 0x38 0x8 */
> > /* --- cacheline 1 boundary (64 bytes) --- */
> > struct red_parms * red_parms; /* 0x40 0x8 */
> > struct tc_sfqred_stats stats; /* 0x48 0x18 */
> > struct sfq_slot * tail; /* 0x60 0x8 */
> > struct sfq_head dep[128]; /* 0x68 0x200 */
> > /* --- cacheline 9 boundary (576 bytes) was 40 bytes ago --- */
> > unsigned int maxflows; /* 0x268 0x4 */
> > int perturb_period; /* 0x26c 0x4 */
> > struct timer_list perturb_timer; /* 0x270 0x28 */
> >
> > /* XXX last struct has 4 bytes of padding */
> >
> > /* --- cacheline 10 boundary (640 bytes) was 24 bytes ago --- */
> > struct Qdisc * sch; /* 0x298 0x8 */
> >
> > /* size: 672, cachelines: 11, members: 20 */
> > /* paddings: 1, sum paddings: 4 */
> > /* last cacheline: 32 bytes */
> > };
> >
> >
> > With this patch :
> >
> > diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
> > index 65d5b59da583..f8fec2bc0d25 100644
> > --- a/net/sched/sch_sfq.c
> > +++ b/net/sched/sch_sfq.c
> > @@ -110,10 +110,11 @@ struct sfq_sched_data {
> > unsigned int divisor; /* number of slots in hash table */
> > u8 headdrop;
> > u8 maxdepth; /* limit of packets per flow */
> > -
> > - siphash_key_t perturbation;
> > u8 cur_depth; /* depth of longest slot */
> > u8 flags;
> > + unsigned int quantum; /* Allotment per round: MUST
> > BE >= MTU */
> > +
> > + siphash_key_t perturbation;
> > struct tcf_proto __rcu *filter_list;
> > struct tcf_block *block;
> > sfq_index *ht; /* Hash table ('divisor' slots) */
> > @@ -132,7 +133,6 @@ struct sfq_sched_data {
> >
> > unsigned int maxflows; /* number of flows in flows array */
> > int perturb_period;
>
> Would it make any sense to additionally move 'maxflows' and
> 'perturb_period' at the top, just after 'perturbation'?
>
> Thanks,
>
> Paolo
>
Powered by blists - more mailing lists