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

[Paolo Abeni <pabeni@redhat.com>]

On 3/16/25 4:27 PM, chia-yu.chang@nokia-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