netdev - Re: [PATCH net 1/3] net_sched: sch_sfq: use a temporary work area for validating configuration

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <18c69469-5357-422e-a7dd-9722d502fd95@redhat.com>
Date: Tue, 1 Apr 2025 13:13:35 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Cong Wang <xiyou.wangcong@...il.com>, Octavian Purdila
 <tavip@...gle.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 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.

> 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