Re: [PATCH net-next v3 1/5] net/sched: Export mq functions for reuse

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Toke Høiland-Jørgensen wrote:
> To enable the cake_mq qdisc to reuse code from the mq qdisc, export a
> bunch of functions from sch_mq. Split common functionality out from some
> functions so it can be composed with other code, and export other
> functions wholesale.
> 
> No functional change intended.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  include/net/sch_generic.h | 19 +++++++++++++
>  net/sched/sch_mq.c        | 69 ++++++++++++++++++++++++++++++++---------------
>  2 files changed, 67 insertions(+), 21 deletions(-)
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index c3a7268b567e..f2281914d962 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h

We probably want to avoid random users. This may be better suited to a
local header, similar to net/core/devmem.h.

I don't mean to derail this feature if these are the only concerns.
This can be revised later in -rcX too.

> @@ -1419,7 +1419,26 @@ void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
>  void mini_qdisc_pair_block_init(struct mini_Qdisc_pair *miniqp,
>  				struct tcf_block *block);
>  
> +struct mq_sched {
> +	struct Qdisc		**qdiscs;
> +};
> +
> +int mq_init_common(struct Qdisc *sch, struct nlattr *opt,
> +		   struct netlink_ext_ack *extack,
> +		   const struct Qdisc_ops *qdisc_ops);
> +void mq_destroy_common(struct Qdisc *sch);
> +void mq_attach(struct Qdisc *sch);
>  void mq_change_real_num_tx(struct Qdisc *sch, unsigned int new_real_tx);
> +void mq_dump_common(struct Qdisc *sch, struct sk_buff *skb);
> +struct netdev_queue *mq_select_queue(struct Qdisc *sch,
> +				     struct tcmsg *tcm);
> +struct Qdisc *mq_leaf(struct Qdisc *sch, unsigned long cl);
> +unsigned long mq_find(struct Qdisc *sch, u32 classid);
> +int mq_dump_class(struct Qdisc *sch, unsigned long cl,
> +		  struct sk_buff *skb, struct tcmsg *tcm);
> +int mq_dump_class_stats(struct Qdisc *sch, unsigned long cl,
> +			struct gnet_dump *d);
> +void mq_walk(struct Qdisc *sch, struct qdisc_walker *arg);
>  
>  int sch_frag_xmit_hook(struct sk_buff *skb, int (*xmit)(struct sk_buff *skb));
>  
> diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
> index c860119a8f09..0bcabdcd1f44 100644
> --- a/net/sched/sch_mq.c
> +++ b/net/sched/sch_mq.c
> @@ -17,10 +17,6 @@
>  #include <net/pkt_sched.h>
>  #include <net/sch_generic.h>
>  
> -struct mq_sched {
> -	struct Qdisc		**qdiscs;
> -};
> -
>  static int mq_offload(struct Qdisc *sch, enum tc_mq_command cmd)
>  {
>  	struct net_device *dev = qdisc_dev(sch);
> @@ -49,23 +45,29 @@ static int mq_offload_stats(struct Qdisc *sch)
>  	return qdisc_offload_dump_helper(sch, TC_SETUP_QDISC_MQ, &opt);
>  }
>  
> -static void mq_destroy(struct Qdisc *sch)
> +void mq_destroy_common(struct Qdisc *sch)
>  {
>  	struct net_device *dev = qdisc_dev(sch);
>  	struct mq_sched *priv = qdisc_priv(sch);
>  	unsigned int ntx;
>  
> -	mq_offload(sch, TC_MQ_DESTROY);
> -
>  	if (!priv->qdiscs)
>  		return;
>  	for (ntx = 0; ntx < dev->num_tx_queues && priv->qdiscs[ntx]; ntx++)
>  		qdisc_put(priv->qdiscs[ntx]);
>  	kfree(priv->qdiscs);
>  }
> +EXPORT_SYMBOL(mq_destroy_common);

On a similar note, this would be a good use of EXPORT_SYMBOL_NS_GPL.

Maybe not even NETDEV_INTERNAL but a dedicated NET_SCHED_MQ.

> -static int mq_init(struct Qdisc *sch, struct nlattr *opt,
> -		   struct netlink_ext_ack *extack)
> +static void mq_destroy(struct Qdisc *sch)
> +{
> +	mq_offload(sch, TC_MQ_DESTROY);
> +	mq_destroy_common(sch);
> +}
> +
> +int mq_init_common(struct Qdisc *sch, struct nlattr *opt,
> +		   struct netlink_ext_ack *extack,
> +		   const struct Qdisc_ops *qdisc_ops)
>  {
>  	struct net_device *dev = qdisc_dev(sch);
>  	struct mq_sched *priv = qdisc_priv(sch);
> @@ -87,7 +89,8 @@ static int mq_init(struct Qdisc *sch, struct nlattr *opt,
>  
>  	for (ntx = 0; ntx < dev->num_tx_queues; ntx++) {
>  		dev_queue = netdev_get_tx_queue(dev, ntx);
> -		qdisc = qdisc_create_dflt(dev_queue, get_default_qdisc_ops(dev, ntx),
> +		qdisc = qdisc_create_dflt(dev_queue,
> +					  qdisc_ops ?: get_default_qdisc_ops(dev, ntx),
>  					  TC_H_MAKE(TC_H_MAJ(sch->handle),
>  						    TC_H_MIN(ntx + 1)),
>  					  extack);
> @@ -98,12 +101,24 @@ static int mq_init(struct Qdisc *sch, struct nlattr *opt,
>  	}
>  
>  	sch->flags |= TCQ_F_MQROOT;
> +	return 0;
> +}
> +EXPORT_SYMBOL(mq_init_common);
> +
> +static int mq_init(struct Qdisc *sch, struct nlattr *opt,
> +		   struct netlink_ext_ack *extack)
> +{
> +	int ret;
> +
> +	ret = mq_init_common(sch, opt, extack, NULL);
> +	if (ret)
> +		return ret;
>  
>  	mq_offload(sch, TC_MQ_CREATE);
>  	return 0;
>  }
>  
> -static void mq_attach(struct Qdisc *sch)
> +void mq_attach(struct Qdisc *sch)
>  {
>  	struct net_device *dev = qdisc_dev(sch);
>  	struct mq_sched *priv = qdisc_priv(sch);
> @@ -124,8 +139,9 @@ static void mq_attach(struct Qdisc *sch)
>  	kfree(priv->qdiscs);
>  	priv->qdiscs = NULL;
>  }
> +EXPORT_SYMBOL(mq_attach);
>  
> -static int mq_dump(struct Qdisc *sch, struct sk_buff *skb)
> +void mq_dump_common(struct Qdisc *sch, struct sk_buff *skb)
>  {
>  	struct net_device *dev = qdisc_dev(sch);
>  	struct Qdisc *qdisc;
> @@ -152,7 +168,12 @@ static int mq_dump(struct Qdisc *sch, struct sk_buff *skb)
>  
>  		spin_unlock_bh(qdisc_lock(qdisc));
>  	}
> +}
> +EXPORT_SYMBOL(mq_dump_common);
>  
> +static int mq_dump(struct Qdisc *sch, struct sk_buff *skb)
> +{
> +	mq_dump_common(sch, skb);
>  	return mq_offload_stats(sch);
>  }
>  
> @@ -166,11 +187,12 @@ static struct netdev_queue *mq_queue_get(struct Qdisc *sch, unsigned long cl)
>  	return netdev_get_tx_queue(dev, ntx);
>  }
>  
> -static struct netdev_queue *mq_select_queue(struct Qdisc *sch,
> -					    struct tcmsg *tcm)
> +struct netdev_queue *mq_select_queue(struct Qdisc *sch,
> +				     struct tcmsg *tcm)
>  {
>  	return mq_queue_get(sch, TC_H_MIN(tcm->tcm_parent));
>  }
> +EXPORT_SYMBOL(mq_select_queue);
>  
>  static int mq_graft(struct Qdisc *sch, unsigned long cl, struct Qdisc *new,
>  		    struct Qdisc **old, struct netlink_ext_ack *extack)
> @@ -198,14 +220,15 @@ static int mq_graft(struct Qdisc *sch, unsigned long cl, struct Qdisc *new,
>  	return 0;
>  }
>  
> -static struct Qdisc *mq_leaf(struct Qdisc *sch, unsigned long cl)
> +struct Qdisc *mq_leaf(struct Qdisc *sch, unsigned long cl)
>  {
>  	struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
>  
>  	return rtnl_dereference(dev_queue->qdisc_sleeping);
>  }
> +EXPORT_SYMBOL(mq_leaf);
>  
> -static unsigned long mq_find(struct Qdisc *sch, u32 classid)
> +unsigned long mq_find(struct Qdisc *sch, u32 classid)
>  {
>  	unsigned int ntx = TC_H_MIN(classid);
>  
> @@ -213,9 +236,10 @@ static unsigned long mq_find(struct Qdisc *sch, u32 classid)
>  		return 0;
>  	return ntx;
>  }
> +EXPORT_SYMBOL(mq_find);
>  
> -static int mq_dump_class(struct Qdisc *sch, unsigned long cl,
> -			 struct sk_buff *skb, struct tcmsg *tcm)
> +int mq_dump_class(struct Qdisc *sch, unsigned long cl,
> +		  struct sk_buff *skb, struct tcmsg *tcm)
>  {
>  	struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
>  
> @@ -224,9 +248,10 @@ static int mq_dump_class(struct Qdisc *sch, unsigned long cl,
>  	tcm->tcm_info = rtnl_dereference(dev_queue->qdisc_sleeping)->handle;
>  	return 0;
>  }
> +EXPORT_SYMBOL(mq_dump_class);
>  
> -static int mq_dump_class_stats(struct Qdisc *sch, unsigned long cl,
> -			       struct gnet_dump *d)
> +int mq_dump_class_stats(struct Qdisc *sch, unsigned long cl,
> +			struct gnet_dump *d)
>  {
>  	struct netdev_queue *dev_queue = mq_queue_get(sch, cl);
>  
> @@ -236,8 +261,9 @@ static int mq_dump_class_stats(struct Qdisc *sch, unsigned long cl,
>  		return -1;
>  	return 0;
>  }
> +EXPORT_SYMBOL(mq_dump_class_stats);
>  
> -static void mq_walk(struct Qdisc *sch, struct qdisc_walker *arg)
> +void mq_walk(struct Qdisc *sch, struct qdisc_walker *arg)
>  {
>  	struct net_device *dev = qdisc_dev(sch);
>  	unsigned int ntx;
> @@ -251,6 +277,7 @@ static void mq_walk(struct Qdisc *sch, struct qdisc_walker *arg)
>  			break;
>  	}
>  }
> +EXPORT_SYMBOL(mq_walk);
>  
>  static const struct Qdisc_class_ops mq_class_ops = {
>  	.select_queue	= mq_select_queue,
> 
> -- 
> 2.52.0
>