Re: [Patch net-next 2/3] net_sched: plug in qdisc ops change_tx_queue_len

[John Fastabend <john.fastabend@gmail.com>]

On 01/19/2018 03:09 PM, Cong Wang wrote:
> Introduce a new qdisc ops ->change_tx_queue_len() so that
> each qdisc could decide how to implement this if it wants.
> Previously we simply read dev->tx_queue_len, after pfifo_fast
> switches to skb array, we need this API to resize the skb array
> when we change dev->tx_queue_len.
> 
> To avoid handling race conditions with TX BH, we need to
> deactivate all TX queues before change the value and bring them
> back after we are done, this also makes implementation easier.
> 
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  include/net/sch_generic.h |  2 ++
>  net/core/dev.c            |  1 +
>  net/sched/sch_generic.c   | 22 ++++++++++++++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index cd1be1f25c36..aae1baa1c30f 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -200,6 +200,7 @@ struct Qdisc_ops {
>  					  struct nlattr *arg,
>  					  struct netlink_ext_ack *extack);
>  	void			(*attach)(struct Qdisc *sch);
> +	void			(*change_tx_queue_len)(struct Qdisc *, unsigned int);
>  
>  	int			(*dump)(struct Qdisc *, struct sk_buff *);
>  	int			(*dump_stats)(struct Qdisc *, struct gnet_dump *);
> @@ -488,6 +489,7 @@ void qdisc_class_hash_remove(struct Qdisc_class_hash *,
>  void qdisc_class_hash_grow(struct Qdisc *, struct Qdisc_class_hash *);
>  void qdisc_class_hash_destroy(struct Qdisc_class_hash *);
>  
> +void dev_qdisc_change_tx_queue_len(struct net_device *dev);
>  void dev_init_scheduler(struct net_device *dev);
>  void dev_shutdown(struct net_device *dev);
>  void dev_activate(struct net_device *dev);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 99d353e4cbb2..24809d858a64 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7058,6 +7058,7 @@ int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
>  			dev->tx_queue_len = orig_len;
>  			return res;
>  		}
> +		dev_qdisc_change_tx_queue_len(dev);
>  	}
>  
>  	return 0;
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index ef8b4ecde2ac..30aaeb3c1bf1 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1178,6 +1178,28 @@ void dev_deactivate(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(dev_deactivate);
>  
> +static void qdisc_change_tx_queue_len(struct net_device *dev,
> +				      struct netdev_queue *dev_queue,
> +				      void *unused)
> +{
> +	struct Qdisc *qdisc = dev_queue->qdisc_sleeping;
> +	const struct Qdisc_ops *ops = qdisc->ops;
> +
> +	if (ops->change_tx_queue_len)
> +		ops->change_tx_queue_len(qdisc, dev->tx_queue_len);

hmm what happens if the resize fails in the next patch,

>  
> +static void pfifo_fast_change_tx_queue_len(struct Qdisc *sch, unsigned int new_len)
> +{
> +	struct pfifo_fast_priv *priv = qdisc_priv(sch);
> +	int prio;
> +
> +	for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
> +		struct skb_array *q = band2list(priv, prio);
> +
> +		skb_array_resize(q, new_len, GFP_KERNEL);
> +	}
> +}
> +

Here skb_array_resize() can fail with ENOMEM, do we need to unwind the
change and push the error up the stack?

Thanks,
John