netdev - Re: [PATCH v9 net-next 3/3] sched: Add dualpi2 qdisc

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] [day] [month] [year] [list]

Message-ID: <26f2f366-aa14-4879-978a-58d46f9d83a4@redhat.com>
Date: Fri, 21 Mar 2025 22:08:13 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: chia-yu.chang@...ia-bell-labs.com, netdev@...r.kernel.org,
 dave.taht@...il.com, jhs@...atatu.com, kuba@...nel.org,
 stephen@...workplumber.org, xiyou.wangcong@...il.com, jiri@...nulli.us,
 davem@...emloft.net, edumazet@...gle.com, horms@...nel.org,
 andrew+netdev@...n.ch, donald.hunter@...il.com, ast@...erby.net,
 liuhangbin@...il.com, shuah@...nel.org, linux-kselftest@...r.kernel.org,
 ij@...nel.org, ncardwell@...gle.com, koen.de_schepper@...ia-bell-labs.com,
 g.white@...lelabs.com, ingemar.s.johansson@...csson.com,
 mirja.kuehlewind@...csson.com, cheshire@...le.com, rs.ietf@....at,
 Jason_Livingood@...cast.com, vidhi_goel@...le.com
Cc: Olga Albisser <olga@...isser.org>,
 Olivier Tilmans <olivier.tilmans@...ia.com>,
 Henrik Steen <henrist@...rist.net>, Bob Briscoe <research@...briscoe.net>
Subject: Re: [PATCH v9 net-next 3/3] sched: Add dualpi2 qdisc

On 3/16/25 4:27 PM, chia-yu.chang@...ia-bell-labs.com wrote:
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 0dbfe069a6e3..987686e91919 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -30,6 +30,7 @@
>  #include <asm/byteorder.h>
>  #include <asm/local.h>
>  
> +#include <linux/netdev_features.h>
>  #include <linux/percpu.h>
>  #include <linux/rculist.h>
>  #include <linux/workqueue.h>

I guess this chunck is a left-over... in any case it should be dropped.

[...]
> +struct dualpi2_sched_data {
> +	struct Qdisc *l_queue;	/* The L4S Low latency queue (L-queue) */
> +	struct Qdisc *sch;	/* The Classic queue (C-queue) */
> +
> +	/* Registered tc filters */
> +	struct {
> +		struct tcf_proto __rcu *filters;
> +		struct tcf_block *block;
> +	} tcf;

This usage of anonimous struct is quite unconventional, the preferred
way of scoping fields is to add apprepriate prefix to the field name.

[...]
> +static bool must_drop(struct Qdisc *sch, struct dualpi2_sched_data *q,
> +		      struct sk_buff *skb)
> +{
> +	u64 local_l_prob;
> +	u32 prob;
> +	bool overload;

Please respect the reverse christmas tree order in variable declaration
(many other occurrences below).

[...]
> +static void drop_and_retry(struct dualpi2_sched_data *q, struct sk_buff *skb,
> +			   struct Qdisc *sch)
> +{
> +	++q->deferred_drops.cnt;
> +	q->deferred_drops.len += qdisc_pkt_len(skb);
> +	consume_skb(skb);

Possibly you want to use kfree_skb_reason() here.

I think this patch is still too huge to get a reasonable review. You
should try to split it further, e.g.  struct definition and parsing in
pX, dump in pX+1, enqueue related ops in p+2, ...

Thanks,

Paolo