From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 9706C200 for ; Thu, 10 May 2018 13:15:55 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 53697B40067; Thu, 10 May 2018 11:15:53 +0000 (UTC) Received: from [192.168.38.17] (84.52.114.114) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Thu, 10 May 2018 12:15:47 +0100 To: Thomas Monjalon , CC: Matan Azrad References: <20180509094337.26112-1-thomas@monjalon.net> <20180509224313.27289-1-thomas@monjalon.net> <20180509224313.27289-11-thomas@monjalon.net> From: Andrew Rybchenko Message-ID: <6fd5308e-9566-b220-7ec9-175d82a4aded@solarflare.com> Date: Thu, 10 May 2018 14:15:44 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180509224313.27289-11-thomas@monjalon.net> Content-Language: en-GB X-Originating-IP: [84.52.114.114] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23834.003 X-TM-AS-Result: No--20.891000-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1525950954-3yeujCExru-G Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2 10/11] net/failsafe: fix sub-device ownership race X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 May 2018 11:15:56 -0000 On 05/10/2018 01:43 AM, Thomas Monjalon wrote: > From: Matan Azrad > > There is time between the sub-device port probing by the sub-device PMD > to the sub-device port ownership taking by a fail-safe port. > > In this time, the port is available for the application usage. For > example, the port will be exposed to the applications which use > RTE_ETH_FOREACH_DEV iterator. > > Thus, ownership unaware applications may manage the port in this time > what may cause a lot of problematic behaviors in the fail-safe > sub-device initialization. > > Register to the ethdev NEW event to take the sub-device port ownership > before it becomes exposed to the application. > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD") > Cc: stable@dpdk.org > > Signed-off-by: Matan Azrad > Acked-by: Gaetan Rivet > --- > drivers/net/failsafe/failsafe.c | 22 ++++++++++--- > drivers/net/failsafe/failsafe_eal.c | 56 ++++++++++++++++++++++----------- > drivers/net/failsafe/failsafe_ether.c | 23 ++++++++++++++ > drivers/net/failsafe/failsafe_private.h | 4 +++ > 4 files changed, 82 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c > index fc989c4f5..c9d128de3 100644 > --- a/drivers/net/failsafe/failsafe.c > +++ b/drivers/net/failsafe/failsafe.c > @@ -204,16 +204,25 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) > } > snprintf(priv->my_owner.name, sizeof(priv->my_owner.name), > FAILSAFE_OWNER_NAME); > + DEBUG("Failsafe port %u owner info: %s_%016"PRIX64, dev->data->port_id, > + priv->my_owner.name, priv->my_owner.id); > + ret = rte_eth_dev_callback_register(RTE_ETH_ALL, RTE_ETH_EVENT_NEW, > + failsafe_eth_new_event_callback, > + dev); > + if (ret) { > + ERROR("Failed to register NEW callback"); > + goto free_args; > + } > ret = failsafe_eal_init(dev); > if (ret) > - goto free_args; > + goto unregister_new_callback; > ret = fs_mutex_init(priv); > if (ret) > - goto free_args; > + goto unregister_new_callback; > ret = failsafe_hotplug_alarm_install(dev); > if (ret) { > ERROR("Could not set up plug-in event detection"); > - goto free_args; > + goto unregister_new_callback; > } > mac = &dev->data->mac_addrs[0]; > if (mac_from_arg) { > @@ -226,7 +235,7 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) > mac); > if (ret) { > ERROR("Failed to set default MAC address"); > - goto free_args; > + goto unregister_new_callback; It will fail to apply on next-net since there is cancel_alarm there now. So, this hunk should be simply skipped. > } > } > } else { > @@ -261,6 +270,9 @@ fs_eth_dev_create(struct rte_vdev_device *vdev) > }; > rte_eth_dev_probing_finish(dev); > return 0; > +unregister_new_callback: > + rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW, > + failsafe_eth_new_event_callback, dev); There is cancel_alarm here in next-net. > free_args: > failsafe_args_free(dev); > free_subs: > @@ -280,6 +292,8 @@ fs_rte_eth_free(const char *name) > dev = rte_eth_dev_allocated(name); > if (dev == NULL) > return -ENODEV; > + rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW, > + failsafe_eth_new_event_callback, dev); > ret = failsafe_eal_uninit(dev); > if (ret) > ERROR("Error while uninitializing sub-EAL"); > diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c > index ce767703f..5672f3961 100644 > --- a/drivers/net/failsafe/failsafe_eal.c > +++ b/drivers/net/failsafe/failsafe_eal.c > @@ -18,8 +18,9 @@ fs_ethdev_portid_get(const char *name, uint16_t *port_id) > return -EINVAL; > } > len = strlen(name); > - RTE_ETH_FOREACH_DEV(pid) { > - if (!strncmp(name, rte_eth_devices[pid].device->name, len)) { > + for (pid = 0; pid < RTE_MAX_ETHPORTS; pid++) { > + if (rte_eth_dev_is_valid_port(pid) && > + !strncmp(name, rte_eth_devices[pid].device->name, len)) { > *port_id = pid; > return 0; > } > @@ -41,6 +42,8 @@ fs_bus_init(struct rte_eth_dev *dev) > continue; > da = &sdev->devargs; > if (fs_ethdev_portid_get(da->name, &pid) != 0) { > + struct rte_eth_dev_owner pid_owner; > + > ret = rte_eal_hotplug_add(da->bus->name, > da->name, > da->args); > @@ -55,12 +58,26 @@ fs_bus_init(struct rte_eth_dev *dev) > ERROR("sub_device %d init went wrong", i); > return -ENODEV; > } > + /* > + * The NEW callback tried to take ownership, check > + * whether it succeed or didn't. > + */ > + rte_eth_dev_owner_get(pid, &pid_owner); > + if (pid_owner.id != PRIV(dev)->my_owner.id) { > + INFO("sub_device %d owner(%s_%016"PRIX64") is not my," > + " owner(%s_%016"PRIX64"), will try again later", Frankly speaking I don't understand what and why will change later. > + i, pid_owner.name, pid_owner.id, > + PRIV(dev)->my_owner.name, > + PRIV(dev)->my_owner.id); > + continue; > + } > } else { > + /* The sub-device port was found. */ > char devstr[DEVARGS_MAXLEN] = ""; > struct rte_devargs *probed_da = > rte_eth_devices[pid].device->devargs; > > - /* Take control of device probed by EAL options. */ > + /* Take control of probed device. */ > free(da->args); > memset(da, 0, sizeof(*da)); > if (probed_da != NULL) > @@ -77,22 +94,23 @@ fs_bus_init(struct rte_eth_dev *dev) > } > INFO("Taking control of a probed sub device" > " %d named %s", i, da->name); > - } > - ret = rte_eth_dev_owner_set(pid, &PRIV(dev)->my_owner); > - if (ret < 0) { > - INFO("sub_device %d owner set failed (%s)," > - " will try again later", i, strerror(-ret)); > - continue; > - } else if (strncmp(rte_eth_devices[pid].device->name, da->name, > - strlen(da->name)) != 0) { > - /* > - * The device probably was removed and its port id was > - * reallocated before ownership set. > - */ > - rte_eth_dev_owner_unset(pid, PRIV(dev)->my_owner.id); > - INFO("sub_device %d was probably removed before taking" > - " ownership, will try again later", i); > - continue; > + ret = rte_eth_dev_owner_set(pid, &PRIV(dev)->my_owner); > + if (ret < 0) { > + INFO("sub_device %d owner set failed (%s), " > + "will try again later", i, strerror(-ret)); > + continue; > + } else if (strncmp(rte_eth_devices[pid].device->name, > + da->name, strlen(da->name)) != 0) { > + /* > + * The device probably was removed and its port > + * id was reallocated before ownership set. > + */ > + rte_eth_dev_owner_unset(pid, > + PRIV(dev)->my_owner.id); > + INFO("sub_device %d was removed before taking" > + " ownership, will try again later", i); > + continue; > + } > } > ETH(sdev) = &rte_eth_devices[pid]; > SUB_ID(sdev) = i; > diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c > index b414a7884..ebce87841 100644 > --- a/drivers/net/failsafe/failsafe_ether.c > +++ b/drivers/net/failsafe/failsafe_ether.c > @@ -463,3 +463,26 @@ failsafe_eth_lsc_event_callback(uint16_t port_id __rte_unused, > else > return 0; > } > + > +/* Take sub-device ownership before it becomes exposed to the application. */ > +int > +failsafe_eth_new_event_callback(uint16_t port_id, > + enum rte_eth_event_type event __rte_unused, > + void *cb_arg, void *out __rte_unused) > +{ > + struct rte_eth_dev *fs_dev = cb_arg; > + struct sub_device *sdev; > + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > + size_t len = strlen(dev->device->name); > + uint8_t i; > + > + FOREACH_SUBDEV_STATE(sdev, i, fs_dev, DEV_PARSED) { > + if (sdev->state >= DEV_PROBED) > + continue; > + if (strncmp(sdev->devargs.name, dev->device->name, len) != 0) Shouldn't we ensure that name is exact match instead of checking sdev->devargs.name prefix only. > + continue; > + rte_eth_dev_owner_set(port_id, &PRIV(fs_dev)->my_owner); I think return value should be processed here. If we really ignore it, it should be explained why. > + break; > + } > + return 0; > +} > diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h > index b54f8e336..cd8e0b8c9 100644 > --- a/drivers/net/failsafe/failsafe_private.h > +++ b/drivers/net/failsafe/failsafe_private.h > @@ -220,6 +220,10 @@ int failsafe_eth_rmv_event_callback(uint16_t port_id, > int failsafe_eth_lsc_event_callback(uint16_t port_id, > enum rte_eth_event_type event, > void *cb_arg, void *out); > +int > +failsafe_eth_new_event_callback(uint16_t port_id, > + enum rte_eth_event_type event, > + void *cb_arg, void *out); Return value type and function name should be on the same line in accordance with style used in this file. Just to be consistent. [...]