[PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[xiayu.zhang]

From: Xiayu Zhang <Xiayu.Zhang@mediatek.com>

This patch adds 2 callback functions get_num_tx_queues() and
get_num_rx_queues() to let WWAN network device driver customize its own
TX and RX queue numbers. It gives WWAN driver a chance to implement its
own software strategies, such as TX Qos.

Currently, if WWAN device driver creates default bearer interface when
calling wwan_register_ops(), there will be only 1 TX queue and 1 RX queue
for the WWAN network device. In this case, driver is not able to enlarge
the queue numbers by calling netif_set_real_num_tx_queues() or
netif_set_real_num_rx_queues() to take advantage of the network device's
capability of supporting multiple TX/RX queues.

As for additional interfaces of secondary bearers, if userspace service
doesn't specify the num_tx_queues or num_rx_queues in netlink message or
iproute2 command, there also will be only 1 TX queue and 1 RX queue for
each additional interface. If userspace service specifies the num_tx_queues
and num_rx_queues, however, these numbers could be not able to match the
capabilities of network device.

Besides, userspace service is hard to learn every WWAN network device's
TX/RX queue numbers.

In order to let WWAN driver determine the queue numbers, this patch adds
below callback functions in wwan_ops:
    struct wwan_ops {
        unsigned int priv_size;
        ...
        unsigned int (*get_num_tx_queues)(unsigned int hint_num);
        unsigned int (*get_num_rx_queues)(unsigned int hint_num);
    };

WWAN subsystem uses the input parameters num_tx_queues and num_rx_queues of
wwan_rtnl_alloc() as hint values, and passes the 2 values to the two
callback functions. WWAN device driver should determine the actual numbers
of network device's TX and RX queues according to the hint value and
device's capabilities.

Signed-off-by: Xiayu Zhang <Xiayu.Zhang@mediatek.com>
---
 drivers/net/wwan/wwan_core.c | 25 ++++++++++++++++++++++++-
 include/linux/wwan.h         |  6 ++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index d293ab688044..00095c6987be 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -823,6 +823,7 @@ static struct net_device *wwan_rtnl_alloc(struct nlattr *tb[],
 	struct wwan_device *wwandev = wwan_dev_get_by_name(devname);
 	struct net_device *dev;
 	unsigned int priv_size;
+	unsigned int num_txqs, num_rxqs;
 
 	if (IS_ERR(wwandev))
 		return ERR_CAST(wwandev);
@@ -833,9 +834,31 @@ static struct net_device *wwan_rtnl_alloc(struct nlattr *tb[],
 		goto out;
 	}
 
+	/* let wwan device driver determine TX queue number if it wants */
+	if (wwandev->ops->get_num_tx_queues) {
+		num_txqs = wwandev->ops->get_num_tx_queues(num_tx_queues);
+		if (num_txqs < 1 || num_txqs > 4096) {
+			dev = ERR_PTR(-EINVAL);
+			goto out;
+		}
+	} else {
+		num_txqs = num_tx_queues;
+	}
+
+	/* let wwan device driver determine RX queue number if it wants */
+	if (wwandev->ops->get_num_rx_queues) {
+		num_rxqs = wwandev->ops->get_num_rx_queues(num_rx_queues);
+		if (num_rxqs < 1 || num_rxqs > 4096) {
+			dev = ERR_PTR(-EINVAL);
+			goto out;
+		}
+	} else {
+		num_rxqs = num_rx_queues;
+	}
+
 	priv_size = sizeof(struct wwan_netdev_priv) + wwandev->ops->priv_size;
 	dev = alloc_netdev_mqs(priv_size, ifname, name_assign_type,
-			       wwandev->ops->setup, num_tx_queues, num_rx_queues);
+			       wwandev->ops->setup, num_txqs, num_rxqs);
 
 	if (dev) {
 		SET_NETDEV_DEV(dev, &wwandev->dev);
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 9fac819f92e3..69c0af7ab6af 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -156,6 +156,10 @@ static inline void *wwan_netdev_drvpriv(struct net_device *dev)
  * @setup: set up a new netdev
  * @newlink: register the new netdev
  * @dellink: remove the given netdev
+ * @get_num_tx_queues: determine number of transmit queues
+ *                     to create when creating a new device.
+ * @get_num_rx_queues: determine number of receive queues
+ *                     to create when creating a new device.
  */
 struct wwan_ops {
 	unsigned int priv_size;
@@ -164,6 +168,8 @@ struct wwan_ops {
 		       u32 if_id, struct netlink_ext_ack *extack);
 	void (*dellink)(void *ctxt, struct net_device *dev,
 			struct list_head *head);
+	unsigned int (*get_num_tx_queues)(unsigned int hint_num);
+	unsigned int (*get_num_rx_queues)(unsigned int hint_num);
 };
 
 int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
-- 
2.17.0

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Jakub Kicinski]

On Wed, 8 Dec 2021 12:04:14 +0800 xiayu.zhang@mediatek.com wrote:
> From: Xiayu Zhang <Xiayu.Zhang@mediatek.com>
> 
> This patch adds 2 callback functions get_num_tx_queues() and
> get_num_rx_queues() to let WWAN network device driver customize its own
> TX and RX queue numbers. It gives WWAN driver a chance to implement its
> own software strategies, such as TX Qos.
> 
> Currently, if WWAN device driver creates default bearer interface when
> calling wwan_register_ops(), there will be only 1 TX queue and 1 RX queue
> for the WWAN network device. In this case, driver is not able to enlarge
> the queue numbers by calling netif_set_real_num_tx_queues() or
> netif_set_real_num_rx_queues() to take advantage of the network device's
> capability of supporting multiple TX/RX queues.
> 
> As for additional interfaces of secondary bearers, if userspace service
> doesn't specify the num_tx_queues or num_rx_queues in netlink message or
> iproute2 command, there also will be only 1 TX queue and 1 RX queue for
> each additional interface. If userspace service specifies the num_tx_queues
> and num_rx_queues, however, these numbers could be not able to match the
> capabilities of network device.
> 
> Besides, userspace service is hard to learn every WWAN network device's
> TX/RX queue numbers.
> 
> In order to let WWAN driver determine the queue numbers, this patch adds
> below callback functions in wwan_ops:
>     struct wwan_ops {
>         unsigned int priv_size;
>         ...
>         unsigned int (*get_num_tx_queues)(unsigned int hint_num);
>         unsigned int (*get_num_rx_queues)(unsigned int hint_num);
>     };
> 
> WWAN subsystem uses the input parameters num_tx_queues and num_rx_queues of
> wwan_rtnl_alloc() as hint values, and passes the 2 values to the two
> callback functions. WWAN device driver should determine the actual numbers
> of network device's TX and RX queues according to the hint value and
> device's capabilities.

I'll mark it as an RFC in patchwork, there needs to be an in-tree user
for this code to be merged.

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Sergey Ryazanov]

On Wed, Dec 8, 2021 at 7:04 AM <xiayu.zhang@mediatek.com> wrote:
> This patch adds 2 callback functions get_num_tx_queues() and
> get_num_rx_queues() to let WWAN network device driver customize its own
> TX and RX queue numbers. It gives WWAN driver a chance to implement its
> own software strategies, such as TX Qos.
>
> Currently, if WWAN device driver creates default bearer interface when
> calling wwan_register_ops(), there will be only 1 TX queue and 1 RX queue
> for the WWAN network device. In this case, driver is not able to enlarge
> the queue numbers by calling netif_set_real_num_tx_queues() or
> netif_set_real_num_rx_queues() to take advantage of the network device's
> capability of supporting multiple TX/RX queues.
>
> As for additional interfaces of secondary bearers, if userspace service
> doesn't specify the num_tx_queues or num_rx_queues in netlink message or
> iproute2 command, there also will be only 1 TX queue and 1 RX queue for
> each additional interface. If userspace service specifies the num_tx_queues
> and num_rx_queues, however, these numbers could be not able to match the
> capabilities of network device.
>
> Besides, userspace service is hard to learn every WWAN network device's
> TX/RX queue numbers.
>
> In order to let WWAN driver determine the queue numbers, this patch adds
> below callback functions in wwan_ops:
>     struct wwan_ops {
>         unsigned int priv_size;
>         ...
>         unsigned int (*get_num_tx_queues)(unsigned int hint_num);
>         unsigned int (*get_num_rx_queues)(unsigned int hint_num);
>     };
>
> WWAN subsystem uses the input parameters num_tx_queues and num_rx_queues of
> wwan_rtnl_alloc() as hint values, and passes the 2 values to the two
> callback functions. WWAN device driver should determine the actual numbers
> of network device's TX and RX queues according to the hint value and
> device's capabilities.

As already stated by Jakub, it is hard to understand a new API
suitability without an API user. I will try to describe possible
issues with the proposed API as far as I understand its usage and
possible solutions. Correct me if I am wrong.

There are actually two tasks related to the queues number selection:
1) default queues number selection if the userspace provides no
information about a wishful number of queues;
2) rejecting the new netdev (bearer) creation if a requested number of
queues seems to be invalid.

Your proposal tries to solve both of these tasks with a single hook
that silently increases or decreases the requested number of queues.
This is creative, but seems contradictory to regular RTNL behavior.
RTNL usually selects a correct default value if no value was
requested, or performs what is requested, or explicitly rejects
requested configuration.

You could handle an invalid queues configuration in the .newlink
callback. This callback is even able to return a string error
representation via the extack argument.

As for the default queues number selection it seems better to
implement the RTNL .get_num_rx_queues callback in the WWAN core and
call optional driver specific callback through it. Something like
this:

static unsigned int wwan_rtnl_get_num_tx_queues(struct nlattr *tb[])
{
    const char *devname = nla_data(tb[IFLA_PARENT_DEV_NAME]);
    struct wwan_device *wwandev = wwan_dev_get_by_name(devname);

    return wwandev && wwandev->ops && wwandev->ops->get_num_tx_queues ?
              wwandev->ops->get_num_tx_queues() : 1;
}

static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
    ...
    .get_num_tx_queues = wwan_rtnl_get_num_tx_queues,
};

This way the default queues number selection will be implemented in a
less surprising way.

But to be able to implement this we need to modify the RTNL ops
.get_num_tx_queues/.get_num_rx_queues callback definitions to make
them able to accept the RTM_NEWLINK message attributes. This is not
difficult since the callbacks are implemented only by a few virtual
devices, but can be assumed too intrusive to implement a one feature
for a single subsystem.

> Signed-off-by: Xiayu Zhang <Xiayu.Zhang@mediatek.com>
> ---
>  drivers/net/wwan/wwan_core.c | 25 ++++++++++++++++++++++++-
>  include/linux/wwan.h         |  6 ++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index d293ab688044..00095c6987be 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -823,6 +823,7 @@ static struct net_device *wwan_rtnl_alloc(struct nlattr *tb[],
>         struct wwan_device *wwandev = wwan_dev_get_by_name(devname);
>         struct net_device *dev;
>         unsigned int priv_size;
> +       unsigned int num_txqs, num_rxqs;
>
>         if (IS_ERR(wwandev))
>                 return ERR_CAST(wwandev);
> @@ -833,9 +834,31 @@ static struct net_device *wwan_rtnl_alloc(struct nlattr *tb[],
>                 goto out;
>         }
>
> +       /* let wwan device driver determine TX queue number if it wants */
> +       if (wwandev->ops->get_num_tx_queues) {
> +               num_txqs = wwandev->ops->get_num_tx_queues(num_tx_queues);
> +               if (num_txqs < 1 || num_txqs > 4096) {
> +                       dev = ERR_PTR(-EINVAL);
> +                       goto out;
> +               }
> +       } else {
> +               num_txqs = num_tx_queues;
> +       }
> +
> +       /* let wwan device driver determine RX queue number if it wants */
> +       if (wwandev->ops->get_num_rx_queues) {
> +               num_rxqs = wwandev->ops->get_num_rx_queues(num_rx_queues);
> +               if (num_rxqs < 1 || num_rxqs > 4096) {
> +                       dev = ERR_PTR(-EINVAL);
> +                       goto out;
> +               }
> +       } else {
> +               num_rxqs = num_rx_queues;
> +       }
> +
>         priv_size = sizeof(struct wwan_netdev_priv) + wwandev->ops->priv_size;
>         dev = alloc_netdev_mqs(priv_size, ifname, name_assign_type,
> -                              wwandev->ops->setup, num_tx_queues, num_rx_queues);
> +                              wwandev->ops->setup, num_txqs, num_rxqs);
>
>         if (dev) {
>                 SET_NETDEV_DEV(dev, &wwandev->dev);
> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> index 9fac819f92e3..69c0af7ab6af 100644
> --- a/include/linux/wwan.h
> +++ b/include/linux/wwan.h
> @@ -156,6 +156,10 @@ static inline void *wwan_netdev_drvpriv(struct net_device *dev)
>   * @setup: set up a new netdev
>   * @newlink: register the new netdev
>   * @dellink: remove the given netdev
> + * @get_num_tx_queues: determine number of transmit queues
> + *                     to create when creating a new device.
> + * @get_num_rx_queues: determine number of receive queues
> + *                     to create when creating a new device.
>   */
>  struct wwan_ops {
>         unsigned int priv_size;
> @@ -164,6 +168,8 @@ struct wwan_ops {
>                        u32 if_id, struct netlink_ext_ack *extack);
>         void (*dellink)(void *ctxt, struct net_device *dev,
>                         struct list_head *head);
> +       unsigned int (*get_num_tx_queues)(unsigned int hint_num);
> +       unsigned int (*get_num_rx_queues)(unsigned int hint_num);
>  };
>
>  int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,

-- 
Sergey

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Xiayu Zhang]

Hi Sergey,

Thanks for your constructive inputs, and sorry for late response.

Indeed, I had considered this solution provided by you as well:

   static unsigned int wwan_rtnl_get_num_tx_queues(struct nlattr *tb[])

   static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
      ...
      .get_num_tx_queues = wwan_rtnl_get_num_tx_queues,
   };

I totally agree that it follows the design of RTNL better.

There are some reasons that let me not apply the solution above, I want
to share them with you. Please correct me if I'm wrong.

   1) in rtnl_create_link, RTNL always prefers to use the number
   provided by userspace service rather than the number returned by
   get_num_tx/rx_queues() of WWAN Core:

      if (tb[IFLA_NUM_TX_QUEUES])
         num_tx_queues = nla_get_u32(tb[IFLA_NUM_TX_QUEUES]);
      else if (ops->get_num_tx_queues)
         num_tx_queues = ops->get_num_tx_queues();

   Although WWAN driver could reject the number selected by userspace
   service in newlink function, this will require userspace service to
   learn this error and implement its retry machanisms. Of course, even
   so, that's not bad.

   I think it's probably better to let WWAN device driver determine
   its default queue number.

   2) As you described, above solution will modify the definition and
   usage of get_num_tx_queues() and get_num_rx_queues() in
   rtnl_link_ops. Userspace service also needs to add new NETLINK
   attributes.

   3) WWAN subsystem implements the rtnl_link_ops and plays a role of
   the bridge between RTNL and WWAN device driver. As a separate
   subsystem, I think it could be able to supply its own callback
   functions to WWAN device driver in wwan_ops just as shown in this
   patch.

   In addition to these reasons, I also agree with your points:
      "can be assumed too intrusive to implement a one feature for 
      a single subsystem."

Please review my thoughts and give me some inputs at your convenience.


On Fri, 2021-12-10 at 02:11 +0300, Sergey Ryazanov wrote:
> On Wed, Dec 8, 2021 at 7:04 AM <xiayu.zhang@mediatek.com> wrote:
> > This patch adds 2 callback functions get_num_tx_queues() and
> > get_num_rx_queues() to let WWAN network device driver customize its
> > own
> > TX and RX queue numbers. It gives WWAN driver a chance to implement
> > its
> > own software strategies, such as TX Qos.
> > 
> > Currently, if WWAN device driver creates default bearer interface
> > when
> > calling wwan_register_ops(), there will be only 1 TX queue and 1 RX
> > queue
> > for the WWAN network device. In this case, driver is not able to
> > enlarge
> > the queue numbers by calling netif_set_real_num_tx_queues() or
> > netif_set_real_num_rx_queues() to take advantage of the network
> > device's
> > capability of supporting multiple TX/RX queues.
> > 
> > As for additional interfaces of secondary bearers, if userspace
> > service
> > doesn't specify the num_tx_queues or num_rx_queues in netlink
> > message or
> > iproute2 command, there also will be only 1 TX queue and 1 RX queue
> > for
> > each additional interface. If userspace service specifies the
> > num_tx_queues
> > and num_rx_queues, however, these numbers could be not able to
> > match the
> > capabilities of network device.
> > 
> > Besides, userspace service is hard to learn every WWAN network
> > device's
> > TX/RX queue numbers.
> > 
> > In order to let WWAN driver determine the queue numbers, this patch
> > adds
> > below callback functions in wwan_ops:
> >     struct wwan_ops {
> >         unsigned int priv_size;
> >         ...
> >         unsigned int (*get_num_tx_queues)(unsigned int hint_num);
> >         unsigned int (*get_num_rx_queues)(unsigned int hint_num);
> >     };
> > 
> > WWAN subsystem uses the input parameters num_tx_queues and
> > num_rx_queues of
> > wwan_rtnl_alloc() as hint values, and passes the 2 values to the
> > two
> > callback functions. WWAN device driver should determine the actual
> > numbers
> > of network device's TX and RX queues according to the hint value
> > and
> > device's capabilities.
> 
> As already stated by Jakub, it is hard to understand a new API
> suitability without an API user. I will try to describe possible
> issues with the proposed API as far as I understand its usage and
> possible solutions. Correct me if I am wrong.
> 
> There are actually two tasks related to the queues number selection:
> 1) default queues number selection if the userspace provides no
> information about a wishful number of queues;
> 2) rejecting the new netdev (bearer) creation if a requested number
> of
> queues seems to be invalid.
> 
> Your proposal tries to solve both of these tasks with a single hook
> that silently increases or decreases the requested number of queues.
> This is creative, but seems contradictory to regular RTNL behavior.
> RTNL usually selects a correct default value if no value was
> requested, or performs what is requested, or explicitly rejects
> requested configuration.
> 
> You could handle an invalid queues configuration in the .newlink
> callback. This callback is even able to return a string error
> representation via the extack argument.
> 
> As for the default queues number selection it seems better to
> implement the RTNL .get_num_rx_queues callback in the WWAN core and
> call optional driver specific callback through it. Something like
> this:
> 
> static unsigned int wwan_rtnl_get_num_tx_queues(struct nlattr *tb[])
> {
>     const char *devname = nla_data(tb[IFLA_PARENT_DEV_NAME]);
>     struct wwan_device *wwandev = wwan_dev_get_by_name(devname);
> 
>     return wwandev && wwandev->ops && wwandev->ops->get_num_tx_queues 
> ?
>               wwandev->ops->get_num_tx_queues() : 1;
> }
> 
> static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
>     ...
>     .get_num_tx_queues = wwan_rtnl_get_num_tx_queues,
> };
> 
> This way the default queues number selection will be implemented in a
> less surprising way.
> 
> But to be able to implement this we need to modify the RTNL ops
> .get_num_tx_queues/.get_num_rx_queues callback definitions to make
> them able to accept the RTM_NEWLINK message attributes. This is not
> difficult since the callbacks are implemented only by a few virtual
> devices, but can be assumed too intrusive to implement a one feature
> for a single subsystem.
> 
> > Signed-off-by: Xiayu Zhang <Xiayu.Zhang@mediatek.com>
> > ---
> >  drivers/net/wwan/wwan_core.c | 25 ++++++++++++++++++++++++-
> >  include/linux/wwan.h         |  6 ++++++
> >  2 files changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wwan/wwan_core.c
> > b/drivers/net/wwan/wwan_core.c
> > index d293ab688044..00095c6987be 100644
> > --- a/drivers/net/wwan/wwan_core.c
> > +++ b/drivers/net/wwan/wwan_core.c
> > @@ -823,6 +823,7 @@ static struct net_device
> > *wwan_rtnl_alloc(struct nlattr *tb[],
> >         struct wwan_device *wwandev =
> > wwan_dev_get_by_name(devname);
> >         struct net_device *dev;
> >         unsigned int priv_size;
> > +       unsigned int num_txqs, num_rxqs;
> > 
> >         if (IS_ERR(wwandev))
> >                 return ERR_CAST(wwandev);
> > @@ -833,9 +834,31 @@ static struct net_device
> > *wwan_rtnl_alloc(struct nlattr *tb[],
> >                 goto out;
> >         }
> > 
> > +       /* let wwan device driver determine TX queue number if it
> > wants */
> > +       if (wwandev->ops->get_num_tx_queues) {
> > +               num_txqs = wwandev->ops-
> > >get_num_tx_queues(num_tx_queues);
> > +               if (num_txqs < 1 || num_txqs > 4096) {
> > +                       dev = ERR_PTR(-EINVAL);
> > +                       goto out;
> > +               }
> > +       } else {
> > +               num_txqs = num_tx_queues;
> > +       }
> > +
> > +       /* let wwan device driver determine RX queue number if it
> > wants */
> > +       if (wwandev->ops->get_num_rx_queues) {
> > +               num_rxqs = wwandev->ops-
> > >get_num_rx_queues(num_rx_queues);
> > +               if (num_rxqs < 1 || num_rxqs > 4096) {
> > +                       dev = ERR_PTR(-EINVAL);
> > +                       goto out;
> > +               }
> > +       } else {
> > +               num_rxqs = num_rx_queues;
> > +       }
> > +
> >         priv_size = sizeof(struct wwan_netdev_priv) + wwandev->ops-
> > >priv_size;
> >         dev = alloc_netdev_mqs(priv_size, ifname, name_assign_type,
> > -                              wwandev->ops->setup, num_tx_queues,
> > num_rx_queues);
> > +                              wwandev->ops->setup, num_txqs,
> > num_rxqs);
> > 
> >         if (dev) {
> >                 SET_NETDEV_DEV(dev, &wwandev->dev);
> > diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> > index 9fac819f92e3..69c0af7ab6af 100644
> > --- a/include/linux/wwan.h
> > +++ b/include/linux/wwan.h
> > @@ -156,6 +156,10 @@ static inline void *wwan_netdev_drvpriv(struct
> > net_device *dev)
> >   * @setup: set up a new netdev
> >   * @newlink: register the new netdev
> >   * @dellink: remove the given netdev
> > + * @get_num_tx_queues: determine number of transmit queues
> > + *                     to create when creating a new device.
> > + * @get_num_rx_queues: determine number of receive queues
> > + *                     to create when creating a new device.
> >   */
> >  struct wwan_ops {
> >         unsigned int priv_size;
> > @@ -164,6 +168,8 @@ struct wwan_ops {
> >                        u32 if_id, struct netlink_ext_ack *extack);
> >         void (*dellink)(void *ctxt, struct net_device *dev,
> >                         struct list_head *head);
> > +       unsigned int (*get_num_tx_queues)(unsigned int hint_num);
> > +       unsigned int (*get_num_rx_queues)(unsigned int hint_num);
> >  };
> > 
> >  int wwan_register_ops(struct device *parent, const struct wwan_ops
> > *ops,
> 
> 

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Sergey Ryazanov]

Hello Xiayu,

On Mon, Dec 13, 2021 at 9:06 AM Xiayu Zhang <xiayu.zhang@mediatek.com> wrote:
> Thanks for your constructive inputs, and sorry for late response.
>
> On Fri, 2021-12-10 at 02:11 +0300, Sergey Ryazanov wrote:
>> On Wed, Dec 8, 2021 at 7:04 AM <xiayu.zhang@mediatek.com> wrote:
>>> This patch adds 2 callback functions get_num_tx_queues() and
>>> get_num_rx_queues() to let WWAN network device driver customize its
>>> own
>>> TX and RX queue numbers. It gives WWAN driver a chance to implement
>>> its
>>> own software strategies, such as TX Qos.
>>>
>>> Currently, if WWAN device driver creates default bearer interface
>>> when
>>> calling wwan_register_ops(), there will be only 1 TX queue and 1 RX
>>> queue
>>> for the WWAN network device. In this case, driver is not able to
>>> enlarge
>>> the queue numbers by calling netif_set_real_num_tx_queues() or
>>> netif_set_real_num_rx_queues() to take advantage of the network
>>> device's
>>> capability of supporting multiple TX/RX queues.
>>>
>>> As for additional interfaces of secondary bearers, if userspace
>>> service
>>> doesn't specify the num_tx_queues or num_rx_queues in netlink
>>> message or
>>> iproute2 command, there also will be only 1 TX queue and 1 RX queue
>>> for
>>> each additional interface. If userspace service specifies the
>>> num_tx_queues
>>> and num_rx_queues, however, these numbers could be not able to
>>> match the
>>> capabilities of network device.
>>>
>>> Besides, userspace service is hard to learn every WWAN network
>>> device's
>>> TX/RX queue numbers.
>>>
>>> In order to let WWAN driver determine the queue numbers, this patch
>>> adds
>>> below callback functions in wwan_ops:
>>>     struct wwan_ops {
>>>         unsigned int priv_size;
>>>         ...
>>>         unsigned int (*get_num_tx_queues)(unsigned int hint_num);
>>>         unsigned int (*get_num_rx_queues)(unsigned int hint_num);
>>>     };
>>>
>>> WWAN subsystem uses the input parameters num_tx_queues and
>>> num_rx_queues of
>>> wwan_rtnl_alloc() as hint values, and passes the 2 values to the
>>> two
>>> callback functions. WWAN device driver should determine the actual
>>> numbers
>>> of network device's TX and RX queues according to the hint value
>>> and
>>> device's capabilities.
>>
>> As already stated by Jakub, it is hard to understand a new API
>> suitability without an API user. I will try to describe possible
>> issues with the proposed API as far as I understand its usage and
>> possible solutions. Correct me if I am wrong.
>>
>> There are actually two tasks related to the queues number selection:
>> 1) default queues number selection if the userspace provides no
>> information about a wishful number of queues;
>> 2) rejecting the new netdev (bearer) creation if a requested number
>> of queues seems to be invalid.
>>
>> Your proposal tries to solve both of these tasks with a single hook
>> that silently increases or decreases the requested number of queues.
>> This is creative, but seems contradictory to regular RTNL behavior.
>> RTNL usually selects a correct default value if no value was
>> requested, or performs what is requested, or explicitly rejects
>> requested configuration.
>>
>> You could handle an invalid queues configuration in the .newlink
>> callback. This callback is even able to return a string error
>> representation via the extack argument.
>>
>> As for the default queues number selection it seems better to
>> implement the RTNL .get_num_rx_queues callback in the WWAN core and
>> call optional driver specific callback through it. Something like
>> this:
>>
>> static unsigned int wwan_rtnl_get_num_tx_queues(struct nlattr *tb[])
>> {
>>     const char *devname = nla_data(tb[IFLA_PARENT_DEV_NAME]);
>>     struct wwan_device *wwandev = wwan_dev_get_by_name(devname);
>>
>>     return wwandev && wwandev->ops && wwandev->ops->get_num_tx_queues
>> ?
>>               wwandev->ops->get_num_tx_queues() : 1;
>> }
>>
>> static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
>>     ...
>>     .get_num_tx_queues = wwan_rtnl_get_num_tx_queues,
>> };
>>
>> This way the default queues number selection will be implemented in a
>> less surprising way.
>>
>> But to be able to implement this we need to modify the RTNL ops
>> .get_num_tx_queues/.get_num_rx_queues callback definitions to make
>> them able to accept the RTM_NEWLINK message attributes. This is not
>> difficult since the callbacks are implemented only by a few virtual
>> devices, but can be assumed too intrusive to implement a one feature
>> for a single subsystem.
>
> Indeed, I had considered this solution provided by you as well:
>
>    static unsigned int wwan_rtnl_get_num_tx_queues(struct nlattr *tb[])
>
>    static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
>       ...
>       .get_num_tx_queues = wwan_rtnl_get_num_tx_queues,
>    };
>
> I totally agree that it follows the design of RTNL better.
>
> There are some reasons that let me not apply the solution above, I want
> to share them with you. Please correct me if I'm wrong.
>
>    1) in rtnl_create_link, RTNL always prefers to use the number
>    provided by userspace service rather than the number returned by
>    get_num_tx/rx_queues() of WWAN Core:
>
>       if (tb[IFLA_NUM_TX_QUEUES])
>          num_tx_queues = nla_get_u32(tb[IFLA_NUM_TX_QUEUES]);
>       else if (ops->get_num_tx_queues)
>          num_tx_queues = ops->get_num_tx_queues();
>
>    Although WWAN driver could reject the number selected by userspace
>    service in newlink function, this will require userspace service to
>    learn this error and implement its retry machanisms. Of course, even
>    so, that's not bad.

Why do you assume that a userspace service must provide the
IFLA_NUM_TX_QUEUES attribute?

This attribute is optional, see below.

>    I think it's probably better to let WWAN device driver determine
>    its default queue number.

Exactly! If we provide RTNL with a .get_num_tx_queues() callback, then
in case of missed IFLA_NUM_TX_QUEUES attribute, RTNL will select the
number of queues according to the driver decision. And only if
userspace forces the driver to use a particular number of queues using
the IFLA_NUM_TX_QUEUES attribute, then RTNL will try to use a
non-default queues number. In that case, the driver may reject the
creation of such a bearer.

So, with the .get_num_tx_queues() callback we will have a simple
scheme. Either, userspace does not specify the IFLA_NUM_TX_QUEUES
attribute and allows the driver to select an appropriate number of
queues. Or, userspace would like to force a specific number of queues
using the IFLA_NUM_TX_QUEUES attribute, but in that case, the
userspace application should be ready to receive a rejection.

>    2) As you described, above solution will modify the definition and
>    usage of get_num_tx_queues() and get_num_rx_queues() in
>    rtnl_link_ops. Userspace service also needs to add new NETLINK
>    attributes.

What new attributes did you mean?

IFLA_NUM_TX_QUEUES is optional as shown above. The
IFLA_PARENT_DEV_NAME attribute must be provided anyway, otherwise the
WWAN subsystem will not be able to locate a particular driver and the
interface (bearer) creation request will be rejected. Attributes
already are passed to the WWAN subsystem via the .rtnl_alloc()
callback. I suggest to pass the same attributes to the
.get_num_tx_queues() callback that will be called against the same
RTM_NEWLINK message, just slightly earlier.

>    3) WWAN subsystem implements the rtnl_link_ops and plays a role of
>    the bridge between RTNL and WWAN device driver. As a separate
>    subsystem, I think it could be able to supply its own callback
>    functions to WWAN device driver in wwan_ops just as shown in this
>    patch.

Yep, we need a callback to be able to support multi-queue modems. I am
just not happy with a callback that silently tries to improve a user's
choice. And I would like to find a more straightforward solution for
multi-queue support.

>    In addition to these reasons, I also agree with your points:
>       "can be assumed too intrusive to implement a one feature for
>       a single subsystem."

But it looks like we have no choice here other than extending the
.get_num_tx_queues() prototype.

Is there any RTNL guru here who could explain whether it is acceptable
to extend the internal API for a single subsystem?

> Please review my thoughts and give me some inputs at your convenience.
>
>>> Signed-off-by: Xiayu Zhang <Xiayu.Zhang@mediatek.com>
>>> ---
>>>  drivers/net/wwan/wwan_core.c | 25 ++++++++++++++++++++++++-
>>>  include/linux/wwan.h         |  6 ++++++
>>>  2 files changed, 30 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wwan/wwan_core.c
>>> b/drivers/net/wwan/wwan_core.c
>>> index d293ab688044..00095c6987be 100644
>>> --- a/drivers/net/wwan/wwan_core.c
>>> +++ b/drivers/net/wwan/wwan_core.c
>>> @@ -823,6 +823,7 @@ static struct net_device
>>> *wwan_rtnl_alloc(struct nlattr *tb[],
>>>         struct wwan_device *wwandev =
>>> wwan_dev_get_by_name(devname);
>>>         struct net_device *dev;
>>>         unsigned int priv_size;
>>> +       unsigned int num_txqs, num_rxqs;
>>>
>>>         if (IS_ERR(wwandev))
>>>                 return ERR_CAST(wwandev);
>>> @@ -833,9 +834,31 @@ static struct net_device
>>> *wwan_rtnl_alloc(struct nlattr *tb[],
>>>                 goto out;
>>>         }
>>>
>>> +       /* let wwan device driver determine TX queue number if it
>>> wants */
>>> +       if (wwandev->ops->get_num_tx_queues) {
>>> +               num_txqs = wwandev->ops-
>>> >get_num_tx_queues(num_tx_queues);
>>> +               if (num_txqs < 1 || num_txqs > 4096) {
>>> +                       dev = ERR_PTR(-EINVAL);
>>> +                       goto out;
>>> +               }
>>> +       } else {
>>> +               num_txqs = num_tx_queues;
>>> +       }
>>> +
>>> +       /* let wwan device driver determine RX queue number if it
>>> wants */
>>> +       if (wwandev->ops->get_num_rx_queues) {
>>> +               num_rxqs = wwandev->ops-
>>> >get_num_rx_queues(num_rx_queues);
>>> +               if (num_rxqs < 1 || num_rxqs > 4096) {
>>> +                       dev = ERR_PTR(-EINVAL);
>>> +                       goto out;
>>> +               }
>>> +       } else {
>>> +               num_rxqs = num_rx_queues;
>>> +       }
>>> +
>>>         priv_size = sizeof(struct wwan_netdev_priv) + wwandev->ops-
>>> >priv_size;
>>>         dev = alloc_netdev_mqs(priv_size, ifname, name_assign_type,
>>> -                              wwandev->ops->setup, num_tx_queues,
>>> num_rx_queues);
>>> +                              wwandev->ops->setup, num_txqs,
>>> num_rxqs);
>>>
>>>         if (dev) {
>>>                 SET_NETDEV_DEV(dev, &wwandev->dev);
>>> diff --git a/include/linux/wwan.h b/include/linux/wwan.h
>>> index 9fac819f92e3..69c0af7ab6af 100644
>>> --- a/include/linux/wwan.h
>>> +++ b/include/linux/wwan.h
>>> @@ -156,6 +156,10 @@ static inline void *wwan_netdev_drvpriv(struct
>>> net_device *dev)
>>>   * @setup: set up a new netdev
>>>   * @newlink: register the new netdev
>>>   * @dellink: remove the given netdev
>>> + * @get_num_tx_queues: determine number of transmit queues
>>> + *                     to create when creating a new device.
>>> + * @get_num_rx_queues: determine number of receive queues
>>> + *                     to create when creating a new device.
>>>   */
>>>  struct wwan_ops {
>>>         unsigned int priv_size;
>>> @@ -164,6 +168,8 @@ struct wwan_ops {
>>>                        u32 if_id, struct netlink_ext_ack *extack);
>>>         void (*dellink)(void *ctxt, struct net_device *dev,
>>>                         struct list_head *head);
>>> +       unsigned int (*get_num_tx_queues)(unsigned int hint_num);
>>> +       unsigned int (*get_num_rx_queues)(unsigned int hint_num);
>>>  };
>>>
>>>  int wwan_register_ops(struct device *parent, const struct wwan_ops
>>> *ops,

-- 
Sergey

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Xiayu Zhang]

Hi Sergey,

Many thanks for your further information, and please find my inputs
below.


On Wed, 2021-12-15 at 08:23 +0800, Sergey Ryazanov wrote:
> Hello Xiayu,
> 
> On Mon, Dec 13, 2021 at 9:06 AM Xiayu Zhang <xiayu.zhang@mediatek.com
> > wrote:
> > Thanks for your constructive inputs, and sorry for late response.
> > 
> > On Fri, 2021-12-10 at 02:11 +0300, Sergey Ryazanov wrote:
> > > On Wed, Dec 8, 2021 at 7:04 AM <xiayu.zhang@mediatek.com> wrote:
> > > > This patch adds 2 callback functions get_num_tx_queues() and
> > > > get_num_rx_queues() to let WWAN network device driver customize
> > > > its
> > > > own
> > > > TX and RX queue numbers. It gives WWAN driver a chance to
> > > > implement
> > > > its
> > > > own software strategies, such as TX Qos.
> > > > 
> > > > Currently, if WWAN device driver creates default bearer
> > > > interface
> > > > when
> > > > calling wwan_register_ops(), there will be only 1 TX queue and
> > > > 1 RX
> > > > queue
> > > > for the WWAN network device. In this case, driver is not able
> > > > to
> > > > enlarge
> > > > the queue numbers by calling netif_set_real_num_tx_queues() or
> > > > netif_set_real_num_rx_queues() to take advantage of the network
> > > > device's
> > > > capability of supporting multiple TX/RX queues.
> > > > 
> > > > As for additional interfaces of secondary bearers, if userspace
> > > > service
> > > > doesn't specify the num_tx_queues or num_rx_queues in netlink
> > > > message or
> > > > iproute2 command, there also will be only 1 TX queue and 1 RX
> > > > queue
> > > > for
> > > > each additional interface. If userspace service specifies the
> > > > num_tx_queues
> > > > and num_rx_queues, however, these numbers could be not able to
> > > > match the
> > > > capabilities of network device.
> > > > 
> > > > Besides, userspace service is hard to learn every WWAN network
> > > > device's
> > > > TX/RX queue numbers.
> > > > 
> > > > In order to let WWAN driver determine the queue numbers, this
> > > > patch
> > > > adds
> > > > below callback functions in wwan_ops:
> > > >     struct wwan_ops {
> > > >         unsigned int priv_size;
> > > >         ...
> > > >         unsigned int (*get_num_tx_queues)(unsigned int
> > > > hint_num);
> > > >         unsigned int (*get_num_rx_queues)(unsigned int
> > > > hint_num);
> > > >     };
> > > > 
> > > > WWAN subsystem uses the input parameters num_tx_queues and
> > > > num_rx_queues of
> > > > wwan_rtnl_alloc() as hint values, and passes the 2 values to
> > > > the
> > > > two
> > > > callback functions. WWAN device driver should determine the
> > > > actual
> > > > numbers
> > > > of network device's TX and RX queues according to the hint
> > > > value
> > > > and
> > > > device's capabilities.
> > > 
> > > As already stated by Jakub, it is hard to understand a new API
> > > suitability without an API user. I will try to describe possible
> > > issues with the proposed API as far as I understand its usage and
> > > possible solutions. Correct me if I am wrong.
> > > 
> > > There are actually two tasks related to the queues number
> > > selection:
> > > 1) default queues number selection if the userspace provides no
> > > information about a wishful number of queues;
> > > 2) rejecting the new netdev (bearer) creation if a requested
> > > number
> > > of queues seems to be invalid.
> > > 
> > > Your proposal tries to solve both of these tasks with a single
> > > hook
> > > that silently increases or decreases the requested number of
> > > queues.
> > > This is creative, but seems contradictory to regular RTNL
> > > behavior.
> > > RTNL usually selects a correct default value if no value was
> > > requested, or performs what is requested, or explicitly rejects
> > > requested configuration.
> > > 
> > > You could handle an invalid queues configuration in the .newlink
> > > callback. This callback is even able to return a string error
> > > representation via the extack argument.
> > > 
> > > As for the default queues number selection it seems better to
> > > implement the RTNL .get_num_rx_queues callback in the WWAN core
> > > and
> > > call optional driver specific callback through it. Something like
> > > this:
> > > 
> > > static unsigned int wwan_rtnl_get_num_tx_queues(struct nlattr
> > > *tb[])
> > > {
> > >     const char *devname = nla_data(tb[IFLA_PARENT_DEV_NAME]);
> > >     struct wwan_device *wwandev = wwan_dev_get_by_name(devname);
> > > 
> > >     return wwandev && wwandev->ops && wwandev->ops-
> > > >get_num_tx_queues
> > > ?
> > >               wwandev->ops->get_num_tx_queues() : 1;
> > > }
> > > 
> > > static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
> > >     ...
> > >     .get_num_tx_queues = wwan_rtnl_get_num_tx_queues,
> > > };
> > > 
> > > This way the default queues number selection will be implemented
> > > in a
> > > less surprising way.
> > > 
> > > But to be able to implement this we need to modify the RTNL ops
> > > .get_num_tx_queues/.get_num_rx_queues callback definitions to
> > > make
> > > them able to accept the RTM_NEWLINK message attributes. This is
> > > not
> > > difficult since the callbacks are implemented only by a few
> > > virtual
> > > devices, but can be assumed too intrusive to implement a one
> > > feature
> > > for a single subsystem.
> > 
> > Indeed, I had considered this solution provided by you as well:
> > 
> >    static unsigned int wwan_rtnl_get_num_tx_queues(struct nlattr
> > *tb[])
> > 
> >    static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
> >       ...
> >       .get_num_tx_queues = wwan_rtnl_get_num_tx_queues,
> >    };
> > 
> > I totally agree that it follows the design of RTNL better.
> > 
> > There are some reasons that let me not apply the solution above, I
> > want
> > to share them with you. Please correct me if I'm wrong.
> > 
> >    1) in rtnl_create_link, RTNL always prefers to use the number
> >    provided by userspace service rather than the number returned by
> >    get_num_tx/rx_queues() of WWAN Core:
> > 
> >       if (tb[IFLA_NUM_TX_QUEUES])
> >          num_tx_queues = nla_get_u32(tb[IFLA_NUM_TX_QUEUES]);
> >       else if (ops->get_num_tx_queues)
> >          num_tx_queues = ops->get_num_tx_queues();
> > 
> >    Although WWAN driver could reject the number selected by
> > userspace
> >    service in newlink function, this will require userspace service
> > to
> >    learn this error and implement its retry machanisms. Of course,
> > even
> >    so, that's not bad.
> 
> Why do you assume that a userspace service must provide the
> IFLA_NUM_TX_QUEUES attribute?

Yes, these attributes are optional. I noticed this code logic in RTNL
and just raised this case for discussion.

> 
> This attribute is optional, see below.
> 
> >    I think it's probably better to let WWAN device driver determine
> >    its default queue number.
> 
> Exactly! If we provide RTNL with a .get_num_tx_queues() callback,
> then
> in case of missed IFLA_NUM_TX_QUEUES attribute, RTNL will select the
> number of queues according to the driver decision. And only if
> userspace forces the driver to use a particular number of queues
> using
> the IFLA_NUM_TX_QUEUES attribute, then RTNL will try to use a
> non-default queues number. In that case, the driver may reject the
> creation of such a bearer.
> 
> So, with the .get_num_tx_queues() callback we will have a simple
> scheme. Either, userspace does not specify the IFLA_NUM_TX_QUEUES
> attribute and allows the driver to select an appropriate number of
> queues. Or, userspace would like to force a specific number of queues
> using the IFLA_NUM_TX_QUEUES attribute, but in that case, the
> userspace application should be ready to receive a rejection.
> 
> >    2) As you described, above solution will modify the definition
> > and
> >    usage of get_num_tx_queues() and get_num_rx_queues() in
> >    rtnl_link_ops. Userspace service also needs to add new NETLINK
> >    attributes.
> 
> What new attributes did you mean?

Sorry for this misleading description here. I wanted to explain that
it's better to let WWAN driver determine the default queue number. If
userspace service wants to specify the numbers, it needs to add
additional NETLINK attributes for TX/RX queue numbers. I should have
put this into the description of reason 1).

> 
> IFLA_NUM_TX_QUEUES is optional as shown above. The
> IFLA_PARENT_DEV_NAME attribute must be provided anyway, otherwise the
> WWAN subsystem will not be able to locate a particular driver and the
> interface (bearer) creation request will be rejected. Attributes
> already are passed to the WWAN subsystem via the .rtnl_alloc()
> callback. I suggest to pass the same attributes to the
> .get_num_tx_queues() callback that will be called against the same
> RTM_NEWLINK message, just slightly earlier.

Yes. I had tested the same solution before I made this upstream
request. So, I guess I understood your thoughts.

> 
> >    3) WWAN subsystem implements the rtnl_link_ops and plays a role
> > of
> >    the bridge between RTNL and WWAN device driver. As a separate
> >    subsystem, I think it could be able to supply its own callback
> >    functions to WWAN device driver in wwan_ops just as shown in
> > this
> >    patch.
> 
> Yep, we need a callback to be able to support multi-queue modems. I
> am
> just not happy with a callback that silently tries to improve a
> user's
> choice. And I would like to find a more straightforward solution for
> multi-queue support.

Yes, your solution looks more straightforward.

As you said, IFLA_NUM_TX_QUEUES is optional. Userspace service probably
doesn't care or need to learn how many TX/RX queues created by device
driver.

If it wants, it can learn the actual queue numbers from userspace
interfaces (such as sysfs) even driver changed the numbers silently.

> 
> >    In addition to these reasons, I also agree with your points:
> >       "can be assumed too intrusive to implement a one feature for
> >       a single subsystem."
> 
> But it looks like we have no choice here other than extending the
> .get_num_tx_queues() prototype.
> 
> Is there any RTNL guru here who could explain whether it is
> acceptable
> to extend the internal API for a single subsystem?

I want to know whether it is acceptable to add new callback functions
into WWAN subsystem only as well. Hope anyone could help confirm.

> 
> > Please review my thoughts and give me some inputs at your
> > convenience.
> > 
> > > > Signed-off-by: Xiayu Zhang <Xiayu.Zhang@mediatek.com>
> > > > ---
> > > >  drivers/net/wwan/wwan_core.c | 25 ++++++++++++++++++++++++-
> > > >  include/linux/wwan.h         |  6 ++++++
> > > >  2 files changed, 30 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/net/wwan/wwan_core.c
> > > > b/drivers/net/wwan/wwan_core.c
> > > > index d293ab688044..00095c6987be 100644
> > > > --- a/drivers/net/wwan/wwan_core.c
> > > > +++ b/drivers/net/wwan/wwan_core.c
> > > > @@ -823,6 +823,7 @@ static struct net_device
> > > > *wwan_rtnl_alloc(struct nlattr *tb[],
> > > >         struct wwan_device *wwandev =
> > > > wwan_dev_get_by_name(devname);
> > > >         struct net_device *dev;
> > > >         unsigned int priv_size;
> > > > +       unsigned int num_txqs, num_rxqs;
> > > > 
> > > >         if (IS_ERR(wwandev))
> > > >                 return ERR_CAST(wwandev);
> > > > @@ -833,9 +834,31 @@ static struct net_device
> > > > *wwan_rtnl_alloc(struct nlattr *tb[],
> > > >                 goto out;
> > > >         }
> > > > 
> > > > +       /* let wwan device driver determine TX queue number if
> > > > it
> > > > wants */
> > > > +       if (wwandev->ops->get_num_tx_queues) {
> > > > +               num_txqs = wwandev->ops-
> > > > > get_num_tx_queues(num_tx_queues);
> > > > 
> > > > +               if (num_txqs < 1 || num_txqs > 4096) {
> > > > +                       dev = ERR_PTR(-EINVAL);
> > > > +                       goto out;
> > > > +               }
> > > > +       } else {
> > > > +               num_txqs = num_tx_queues;
> > > > +       }
> > > > +
> > > > +       /* let wwan device driver determine RX queue number if
> > > > it
> > > > wants */
> > > > +       if (wwandev->ops->get_num_rx_queues) {
> > > > +               num_rxqs = wwandev->ops-
> > > > > get_num_rx_queues(num_rx_queues);
> > > > 
> > > > +               if (num_rxqs < 1 || num_rxqs > 4096) {
> > > > +                       dev = ERR_PTR(-EINVAL);
> > > > +                       goto out;
> > > > +               }
> > > > +       } else {
> > > > +               num_rxqs = num_rx_queues;
> > > > +       }
> > > > +
> > > >         priv_size = sizeof(struct wwan_netdev_priv) + wwandev-
> > > > >ops-
> > > > > priv_size;
> > > > 
> > > >         dev = alloc_netdev_mqs(priv_size, ifname,
> > > > name_assign_type,
> > > > -                              wwandev->ops->setup,
> > > > num_tx_queues,
> > > > num_rx_queues);
> > > > +                              wwandev->ops->setup, num_txqs,
> > > > num_rxqs);
> > > > 
> > > >         if (dev) {
> > > >                 SET_NETDEV_DEV(dev, &wwandev->dev);
> > > > diff --git a/include/linux/wwan.h b/include/linux/wwan.h
> > > > index 9fac819f92e3..69c0af7ab6af 100644
> > > > --- a/include/linux/wwan.h
> > > > +++ b/include/linux/wwan.h
> > > > @@ -156,6 +156,10 @@ static inline void
> > > > *wwan_netdev_drvpriv(struct
> > > > net_device *dev)
> > > >   * @setup: set up a new netdev
> > > >   * @newlink: register the new netdev
> > > >   * @dellink: remove the given netdev
> > > > + * @get_num_tx_queues: determine number of transmit queues
> > > > + *                     to create when creating a new device.
> > > > + * @get_num_rx_queues: determine number of receive queues
> > > > + *                     to create when creating a new device.
> > > >   */
> > > >  struct wwan_ops {
> > > >         unsigned int priv_size;
> > > > @@ -164,6 +168,8 @@ struct wwan_ops {
> > > >                        u32 if_id, struct netlink_ext_ack
> > > > *extack);
> > > >         void (*dellink)(void *ctxt, struct net_device *dev,
> > > >                         struct list_head *head);
> > > > +       unsigned int (*get_num_tx_queues)(unsigned int
> > > > hint_num);
> > > > +       unsigned int (*get_num_rx_queues)(unsigned int
> > > > hint_num);
> > > >  };
> > > > 
> > > >  int wwan_register_ops(struct device *parent, const struct
> > > > wwan_ops
> > > > *ops,
> 
> -- 
> Sergey

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Loic Poulain]

Hi Xiayu, Sergey,

> > > > As for the default queues number selection it seems better to
> > > > implement the RTNL .get_num_rx_queues callback in the WWAN core
> > > > and
> > > > call optional driver specific callback through it. Something like
> > > > this:
> > > >
> > > > static unsigned int wwan_rtnl_get_num_tx_queues(struct nlattr
> > > > *tb[])
> > > > {
> > > >     const char *devname = nla_data(tb[IFLA_PARENT_DEV_NAME]);
> > > >     struct wwan_device *wwandev = wwan_dev_get_by_name(devname);
> > > >
> > > >     return wwandev && wwandev->ops && wwandev->ops-
> > > > >get_num_tx_queues
> > > > ?
> > > >               wwandev->ops->get_num_tx_queues() : 1;
> > > > }
> > > >
> > > > static struct rtnl_link_ops wwan_rtnl_link_ops __read_mostly = {
> > > >     ...
> > > >     .get_num_tx_queues = wwan_rtnl_get_num_tx_queues,
> > > > };
> > > >
> > > > This way the default queues number selection will be implemented
> > > > in a
> > > > less surprising way.
> > > >
> > > > But to be able to implement this we need to modify the RTNL ops
> > > > .get_num_tx_queues/.get_num_rx_queues callback definitions to
> > > > make
> > > > them able to accept the RTM_NEWLINK message attributes. This is
> > > > not
> > > > difficult since the callbacks are implemented only by a few
> > > > virtual
> > > > devices, but can be assumed too intrusive to implement a one
> > > > feature
> > > > for a single subsystem.

I agree with this solution. The wwan core can forward
get_num_tx_queues to per wwan driver callbacks or simply rely on a
'default_tx_queues' field in ops struct. As said previously, you need
a user for this change in the same series.

Regards,
Loic

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Sergey Ryazanov]

On Wed, Dec 15, 2021 at 11:44 AM Xiayu Zhang <xiayu.zhang@mediatek.com> wrote:
> I want to know whether it is acceptable to add new callback functions
> into WWAN subsystem only as well. Hope anyone could help confirm.

Yes. Feel free to add any callbacks to the WWAN subsystem for proper
hardware support.

There are two things that trigger the discussion:
1) absence of users of the new API;
2) an attempt to silently correct a user choice instead of explicit
rejection of a wrong value.

-- 
Sergey

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Xiayu Zhang]

Hi Sergey and Loic,

Really thank you for these information.

It seems that I need to submit another patch for discussion.

At the meantime, I have some questions below and hope you could do me a
favor.

On Wed, 2021-12-15 at 22:16 +0800, Sergey Ryazanov wrote:

> There are two things that trigger the discussion:
> 1) absence of users of the new API;

Can I choose WWAN device simulator (wwan_hwsim.c) as the in-tree user
for these new APIs? And, Can I submit new APIs and changes for the user
driver in a single patch?

> 2) an attempt to silently correct a user choice instead of explicit
> rejection of a wrong value.

I will try to follow this:
   a. If user doesn't specify a number, use WWAN driver's default
   number.
   b. If user specifies an improper value, reject it explicitly.

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Sergey Ryazanov]

On Thu, Dec 16, 2021 at 3:13 PM Xiayu Zhang <xiayu.zhang@mediatek.com> wrote:
> Hi Sergey and Loic,
>
> Really thank you for these information.
>
> It seems that I need to submit another patch for discussion.
>
> At the meantime, I have some questions below and hope you could do me a
> favor.
>
> On Wed, 2021-12-15 at 22:16 +0800, Sergey Ryazanov wrote:
>
> > There are two things that trigger the discussion:
> > 1) absence of users of the new API;
>
> Can I choose WWAN device simulator (wwan_hwsim.c) as the in-tree user
> for these new APIs? And, Can I submit new APIs and changes for the user
> driver in a single patch?

This is not a good idea. Simulator is intended to test the API that is
used by other drivers for real hardware. But not for experiments with
an otherwise "userless" API.

If you need to configure the number of queues for an already in-tree
driver, then just submit a patch for it. If you plan to submit a new
driver and you need an infrastructure for it, then include patches
with a new API into a series with the driver.

>> 2) an attempt to silently correct a user choice instead of explicit
>> rejection of a wrong value.
>
> I will try to follow this:
>    a. If user doesn't specify a number, use WWAN driver's default
>    number.
>    b. If user specifies an improper value, reject it explicitly.

Yep, this would be a good solution at the moment.

-- 
Sergey

Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

[Xiayu Zhang]

Hi Sergey,

Looks like I can only submit new WWAN APIs until I find a real in-tree
user or submit a new driver.

Anyway, thanks a lot for your help.


On Thu, 2021-12-16 at 20:39 +0800, Sergey Ryazanov wrote:

> This is not a good idea. Simulator is intended to test the API that
> is
> used by other drivers for real hardware. But not for experiments with
> an otherwise "userless" API.
> 
> If you need to configure the number of queues for an already in-tree
> driver, then just submit a patch for it. If you plan to submit a new
> driver and you need an infrastructure for it, then include patches
> with a new API into a series with the driver.