Acked-by: Igor Ryzhov On Sat, Oct 9, 2021 at 2:58 AM Ferruh Yigit 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 > --- > Cc: Igor Ryzhov > Cc: Elad Nachman > Cc: Eric Christian > Cc: Stephen Hemminger > --- > .../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 /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 > >