lkml.org 
[lkml]   [2021]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@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

\
 
 \ /
  Last update: 2021-12-15 01:24    [W:2.262 / U:0.220 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site