netdev - Re: [PATCH net-next v7 3/3] net: add sysfs attribute to control napi threaded mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0UdKXjPM7sf2qKntEZQWgmDq0yfTOtcfevkZFY11kVK4Qg@mail.gmail.com>
Date:   Wed, 20 Jan 2021 08:12:51 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Wei Wang <weiwan@...gle.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>,
        Felix Fietkau <nbd@....name>
Subject: Re: [PATCH net-next v7 3/3] net: add sysfs attribute to control napi
 threaded mode

On Tue, Jan 19, 2021 at 7:35 PM Wei Wang <weiwan@...gle.com> wrote:
>
> This patch adds a new sysfs attribute to the network device class.
> Said attribute provides a per-device control to enable/disable the
> threaded mode for all the napi instances of the given network device.
> User sets it to 1 or 0 to enable or disable threaded mode per device.
> However, when user reads from this sysfs entry, it could return:
>   1: means all napi instances belonging to this device have threaded
> mode enabled.
>   0: means all napi instances belonging to this device have threaded
> mode disabled.
>   -1: means the system fails to enable threaded mode for certain napi
> instances when user requests to enable threaded mode. This happens
> when the kthread fails to be created for certain napi instances.
>
> Co-developed-by: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> Co-developed-by: Hannes Frederic Sowa <hannes@...essinduktion.org>
> Signed-off-by: Hannes Frederic Sowa <hannes@...essinduktion.org>
> Co-developed-by: Felix Fietkau <nbd@....name>
> Signed-off-by: Felix Fietkau <nbd@....name>
> Signed-off-by: Wei Wang <weiwan@...gle.com>
> ---
>  include/linux/netdevice.h |  2 ++
>  net/core/dev.c            | 28 ++++++++++++++++
>  net/core/net-sysfs.c      | 68 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 98 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 8cb8d43ea5fa..26c3e8cf4c01 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -497,6 +497,8 @@ static inline bool napi_complete(struct napi_struct *n)
>         return napi_complete_done(n, 0);
>  }
>
> +int dev_set_threaded(struct net_device *dev, bool threaded);
> +
>  /**
>   *     napi_disable - prevent NAPI from scheduling
>   *     @n: NAPI context
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7ffa91475856..e71c2fd91595 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6767,6 +6767,34 @@ static int napi_set_threaded(struct napi_struct *n, bool threaded)
>         return 0;
>  }
>
> +static void dev_disable_threaded_all(struct net_device *dev)
> +{
> +       struct napi_struct *napi;
> +
> +       list_for_each_entry(napi, &dev->napi_list, dev_list)
> +               napi_set_threaded(napi, false);
> +}
> +
> +int dev_set_threaded(struct net_device *dev, bool threaded)
> +{
> +       struct napi_struct *napi;
> +       int ret;
> +
> +       dev->threaded = threaded;
> +       list_for_each_entry(napi, &dev->napi_list, dev_list) {
> +               ret = napi_set_threaded(napi, threaded);
> +               if (ret) {
> +                       /* Error occurred on one of the napi,
> +                        * reset threaded mode on all napi.
> +                        */
> +                       dev_disable_threaded_all(dev);
> +                       break;
> +               }
> +       }
> +
> +       return ret;
> +}
> +

So I have a question about this function. Is there any reason why
napi_set_threaded couldn't be broken into two pieces and handled in
two passes with the first allocating the kthread and the second
setting the threaded bit assuming the allocations all succeeded? The
setting or clearing of the bit shouldn't need any return value since
it is void and the allocation of the kthread is the piece that can
fail. So it seems like it would make sense to see if you can allocate
all of the kthreads first before you go through and attempt to enable
threaded NAPI.

Then you should only need to make a change to netif_napi_add that will
allocate the kthread if adding a new instance on a device that is
running in threaded mode and if a thread allocation fails you could
clear dev->threaded so that when napi_enable is called we don't bother
enabling any threaded setups since some of the threads are
non-functional.

Doing so would guarantee all-or-nothing behavior and you could then
just use the dev->threaded to signal if the device is running threaded
or not as you could just clear it if the kthread allocation fails.