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: <CANn89iJW0VGQMvq6Bs8co8Bq6Dq1dUT7TN+EXg=GwYbSywUz0A@mail.gmail.com>
Date: Tue, 1 Apr 2025 12:47:11 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Paolo Abeni <pabeni@...hat.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 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 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;
-       unsigned int    quantum;        /* Allotment per round: MUST
BE >= MTU */
        struct timer_list perturb_timer;
        struct Qdisc    *sch;
 };