Date:   Wed, 15 Dec 2021 03:23:28 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     Xiayu Zhang <xiayu.zhang@...iatek.com>
Cc:     Loic Poulain <loic.poulain@...aro.org>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Johannes Berg <johannes@...solutions.net>,
        netdev@...r.kernel.org, open list <linux-kernel@...r.kernel.org>,
        haijun.liu@...iatek.com, zhaoping.shu@...iatek.com,
        hw.he@...iatek.com, srv_heupstream@...iatek.com
Subject: Re: [PATCH] Add Multiple TX/RX Queues Support for WWAN Network Device

Hello Xiayu,

On Mon, Dec 13, 2021 at 9:06 AM Xiayu Zhang <xiayu.zhang@...iatek.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@...iatek.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@...iatek.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

Powered by blists - more mailing lists