Re: [PATCH] kni: restrict bifurcated device support

[Igor Ryzhov <iryzhov@nfware.com>]

[-- Attachment #1: Type: text/plain, Size: 8674 bytes --]

Acked-by: Igor Ryzhov <iryzhov@nfware.com>

On Sat, Oct 9, 2021 at 2:58 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> To enable bifurcated device support, rtnl_lock is released before calling
> userspace callbacks and asynchronous requests are enabled.
>
> But these changes caused more issues, like bug #809, #816. To reduce the
> scope of the problems, the bifurcated device support related changes are
> only enabled when it is requested explicitly with new 'enable_bifurcated'
> module parameter.
> And bifurcated device support is disabled by default.
>
> So the bifurcated device related problems are isolated and they can be
> fixed without impacting all use cases.
>
> Bugzilla ID: 816
> Fixes: 631217c76135 ("kni: fix kernel deadlock with bifurcated device")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Igor Ryzhov <iryzhov@nfware.com>
> Cc: Elad Nachman <eladv6@gmail.com>
> Cc: Eric Christian <erclists@gmail.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  .../prog_guide/kernel_nic_interface.rst       | 28 +++++++++++++
>  kernel/linux/kni/kni_dev.h                    |  3 ++
>  kernel/linux/kni/kni_misc.c                   | 36 ++++++++++++++++
>  kernel/linux/kni/kni_net.c                    | 42 +++++++++++--------
>  4 files changed, 92 insertions(+), 17 deletions(-)
>
> diff --git a/doc/guides/prog_guide/kernel_nic_interface.rst
> b/doc/guides/prog_guide/kernel_nic_interface.rst
> index 1ce03ec1a374..771c7d7fdac4 100644
> --- a/doc/guides/prog_guide/kernel_nic_interface.rst
> +++ b/doc/guides/prog_guide/kernel_nic_interface.rst
> @@ -56,6 +56,12 @@ can be specified when the module is loaded to control
> its behavior:
>                      off   Interfaces will be created with carrier state
> set to off.
>                      on    Interfaces will be created with carrier state
> set to on.
>                       (charp)
> +    parm:           enable_bifurcated: Enable request processing support
> for
> +                    bifurcated drivers, which means releasing rtnl_lock
> before calling
> +                    userspace callback and supporting async requests
> (default=off):
> +                    on    Enable request processing support for
> bifurcated drivers.
> +                     (charp)
> +
>
>  Loading the ``rte_kni`` kernel module without any optional parameters is
>  the typical way a DPDK application gets packets into and out of the kernel
> @@ -174,6 +180,28 @@ To set the default carrier state to *off*:
>  If the ``carrier`` parameter is not specified, the default carrier state
>  of KNI interfaces will be set to *off*.
>
> +.. _kni_bifurcated_device_support:
> +
> +Bifurcated Device Support
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +User callbacks are executed while kernel module holds the ``rtnl`` lock,
> this
> +causes a deadlock when callbacks run control commands on another Linux
> kernel
> +network interface.
> +
> +Bifurcated devices has kernel network driver part and to prevent deadlock
> for
> +them ``enable_bifurcated`` is used.
> +
> +To enable bifurcated device support:
> +
> +.. code-block:: console
> +
> +    # insmod <build_dir>/kernel/linux/kni/rte_kni.ko enable_bifurcated=on
> +
> +Enabling bifurcated device support releases ``rtnl`` lock before calling
> +callback and locks it back after callback. Also enables asynchronous
> request to
> +support callbacks that requires rtnl lock to work (interface down).
> +
>  KNI Creation and Deletion
>  -------------------------
>
> diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h
> index c15da311ba25..e8633486eeb8 100644
> --- a/kernel/linux/kni/kni_dev.h
> +++ b/kernel/linux/kni/kni_dev.h
> @@ -34,6 +34,9 @@
>  /* Default carrier state for created KNI network interfaces */
>  extern uint32_t kni_dflt_carrier;
>
> +/* Request processing support for bifurcated drivers. */
> +extern uint32_t bifurcated_support;
> +
>  /**
>   * A structure describing the private information for a kni device.
>   */
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index 2b464c438113..aae977c187a9 100644
> --- a/kernel/linux/kni/kni_misc.c
> +++ b/kernel/linux/kni/kni_misc.c
> @@ -41,6 +41,10 @@ static uint32_t multiple_kthread_on;
>  static char *carrier;
>  uint32_t kni_dflt_carrier;
>
> +/* Request processing support for bifurcated drivers. */
> +static char *enable_bifurcated;
> +uint32_t bifurcated_support;
> +
>  #define KNI_DEV_IN_USE_BIT_NUM 0 /* Bit number for device in use */
>
>  static int kni_net_id;
> @@ -568,6 +572,22 @@ kni_parse_carrier_state(void)
>         return 0;
>  }
>
> +static int __init
> +kni_parse_bifurcated_support(void)
> +{
> +       if (!enable_bifurcated) {
> +               bifurcated_support = 0;
> +               return 0;
> +       }
> +
> +       if (strcmp(enable_bifurcated, "on") == 0)
> +               bifurcated_support = 1;
> +       else
> +               return -1;
> +
> +       return 0;
> +}
> +
>  static int __init
>  kni_init(void)
>  {
> @@ -593,6 +613,13 @@ kni_init(void)
>         else
>                 pr_debug("Default carrier state set to on.\n");
>
> +       if (kni_parse_bifurcated_support() < 0) {
> +               pr_err("Invalid parameter for bifurcated support\n");
> +               return -EINVAL;
> +       }
> +       if (bifurcated_support == 1)
> +               pr_debug("bifurcated support is enabled.\n");
> +
>  #ifdef HAVE_SIMPLIFIED_PERNET_OPERATIONS
>         rc = register_pernet_subsys(&kni_net_ops);
>  #else
> @@ -659,3 +686,12 @@ MODULE_PARM_DESC(carrier,
>  "\t\ton    Interfaces will be created with carrier state set to on.\n"
>  "\t\t"
>  );
> +
> +module_param(enable_bifurcated, charp, 0644);
> +MODULE_PARM_DESC(enable_bifurcated,
> +"Enable request processing support for bifurcated drivers, "
> +"which means releasing rtnl_lock before calling userspace callback and "
> +"supporting async requests (default=off):\n"
> +"\t\ton    Enable request processing support for bifurcated drivers.\n"
> +"\t\t"
> +);
> diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c
> index 611719b5ee27..29e5b9e21f9e 100644
> --- a/kernel/linux/kni/kni_net.c
> +++ b/kernel/linux/kni/kni_net.c
> @@ -113,12 +113,14 @@ kni_net_process_request(struct net_device *dev,
> struct rte_kni_request *req)
>
>         ASSERT_RTNL();
>
> -       /* If we need to wait and RTNL mutex is held
> -        * drop the mutex and hold reference to keep device
> -        */
> -       if (req->async == 0) {
> -               dev_hold(dev);
> -               rtnl_unlock();
> +       if (bifurcated_support) {
> +               /* If we need to wait and RTNL mutex is held
> +                * drop the mutex and hold reference to keep device
> +                */
> +               if (req->async == 0) {
> +                       dev_hold(dev);
> +                       rtnl_unlock();
> +               }
>         }
>
>         mutex_lock(&kni->sync_lock);
> @@ -132,12 +134,14 @@ kni_net_process_request(struct net_device *dev,
> struct rte_kni_request *req)
>                 goto fail;
>         }
>
> -       /* No result available since request is handled
> -        * asynchronously. set response to success.
> -        */
> -       if (req->async != 0) {
> -               req->result = 0;
> -               goto async;
> +       if (bifurcated_support) {
> +               /* No result available since request is handled
> +                * asynchronously. set response to success.
> +                */
> +               if (req->async != 0) {
> +                       req->result = 0;
> +                       goto async;
> +               }
>         }
>
>         ret_val = wait_event_interruptible_timeout(kni->wq,
> @@ -160,9 +164,11 @@ kni_net_process_request(struct net_device *dev,
> struct rte_kni_request *req)
>
>  fail:
>         mutex_unlock(&kni->sync_lock);
> -       if (req->async == 0) {
> -               rtnl_lock();
> -               dev_put(dev);
> +       if (bifurcated_support) {
> +               if (req->async == 0) {
> +                       rtnl_lock();
> +                       dev_put(dev);
> +               }
>         }
>         return ret;
>  }
> @@ -207,8 +213,10 @@ kni_net_release(struct net_device *dev)
>         /* Setting if_up to 0 means down */
>         req.if_up = 0;
>
> -       /* request async because of the deadlock problem */
> -       req.async = 1;
> +       if (bifurcated_support) {
> +               /* request async because of the deadlock problem */
> +               req.async = 1;
> +       }
>
>         ret = kni_net_process_request(dev, &req);
>
> --
> 2.31.1
>
>

[-- Attachment #2: Type: text/html, Size: 10908 bytes --]