From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luca Boccassi Subject: Re: [PATCH v9 1/5] ethdev: add support of NIC reset Date: Thu, 14 Sep 2017 19:16:11 +0100 Message-ID: <1505412971.30262.11.camel@gmail.com> References: <1500304503-41592-1-git-send-email-wei.dai@intel.com> <1500801313-25429-1-git-send-email-wei.dai@intel.com> <1500801313-25429-2-git-send-email-wei.dai@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org To: Wei Dai , thomas@monjalon.net, wenzhuo.lu@intel.com, konstantin.ananyev@intel.com, jingjing.wu@intel.com, beilei.xing@intel.com Return-path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id 96BD77CCD for ; Thu, 14 Sep 2017 20:16:15 +0200 (CEST) Received: by mail-wm0-f68.google.com with SMTP id r136so821387wmf.3 for ; Thu, 14 Sep 2017 11:16:15 -0700 (PDT) In-Reply-To: <1500801313-25429-2-git-send-email-wei.dai@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Sun, 2017-07-23 at 17:15 +0800, Wei Dai wrote: > This patch adds a new eth_dev layer API function rte_eth_dev_reset(), > which a DPDK application can call to reset a NIC and keep its port id > afterwards. It means that all software resources allocated in the > ethdev > layer are kept, and software & hardware resources of the NIC within > the > NIC's PMD are reset to a state simular to that obtained by calling > the > PCI dev_uninit() and then dev_init(). This effective sequence of > dev_uninit() and dev_init() is packed into a single API function > rte_eth_dev_reset(). >=20 > Please see the comments before the declaration of rte_eht_dev_reset() > in lib/librte_ether/rte_ethdev.h to get more details on why this > function is needed, what it does, when it should be called > and what an application should do after calling this function. >=20 > Signed-off-by: Wei Dai > Reviewed-by: Remy Horton > --- > =C2=A0lib/librte_ether/rte_ethdev.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0| 17 +++++++++++++++++ > =C2=A0lib/librte_ether/rte_ethdev.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0| 34 > ++++++++++++++++++++++++++++++++++ > =C2=A0lib/librte_ether/rte_ether_version.map |=C2=A0=C2=A01 + > =C2=A03 files changed, 52 insertions(+) >=20 > diff --git a/lib/librte_ether/rte_ethdev.c > b/lib/librte_ether/rte_ethdev.c > index d4ebb1b..68ba64d 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -1004,6 +1004,23 @@ rte_eth_dev_close(uint8_t port_id) > =C2=A0} > =C2=A0 > =C2=A0int > +rte_eth_dev_reset(uint8_t port_id) > +{ > + struct rte_eth_dev *dev; > + int ret; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > + dev =3D &rte_eth_devices[port_id]; > + > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_reset, -ENOTSUP); > + > + rte_eth_dev_stop(port_id); > + ret =3D dev->dev_ops->dev_reset(dev); > + > + return ret; > +} > + > +int > =C2=A0rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id, > =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0uint16_t nb_rx_desc, un= signed int socket_id, > =C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0const struct rte_eth_rx= conf *rx_conf, > diff --git a/lib/librte_ether/rte_ethdev.h > b/lib/librte_ether/rte_ethdev.h > index 0e99090..fde92a1 100644 > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -1115,6 +1115,9 @@ typedef int=C2=A0=C2=A0(*eth_dev_set_link_down_t)(s= truct > rte_eth_dev *dev); > =C2=A0typedef void (*eth_dev_close_t)(struct rte_eth_dev *dev); > =C2=A0/**< @internal Function used to close a configured Ethernet device. > */ > =C2=A0 > +typedef int (*eth_dev_reset_t)(struct rte_eth_dev *dev); > +/** <@internal Function used to reset a configured Ethernet device. > */ > + > =C2=A0typedef void (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev); > =C2=A0/**< @internal Function used to enable the RX promiscuous mode of a= n > Ethernet device. */ > =C2=A0 > @@ -1435,6 +1438,7 @@ struct eth_dev_ops { > =C2=A0 eth_dev_set_link_up_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dev_set_l= ink_up;=C2=A0=C2=A0=C2=A0/**< Device > link up. */ > =C2=A0 eth_dev_set_link_down_t=C2=A0=C2=A0=C2=A0=C2=A0dev_set_link_down; = /**< Device > link down. */ > =C2=A0 eth_dev_close_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0dev_close;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/**< Close dev= ice. > */ > + eth_dev_reset_t =C2=A0=C2=A0=C2=A0dev_reset; =C2=A0=C2=A0/**< > Reset device. */ > =C2=A0 eth_link_update_t=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0link_update;=C2=A0=C2=A0=C2=A0/**< Get device > link state. */ > =C2=A0 > =C2=A0 eth_promiscuous_enable_t=C2=A0=C2=A0=C2=A0promiscuous_enable; /**< > Promiscuous ON. */ > @@ -2140,6 +2144,36 @@ int rte_eth_dev_set_link_down(uint8_t > port_id); > =C2=A0void rte_eth_dev_close(uint8_t port_id); > =C2=A0 > =C2=A0/** > + * Reset a Ethernet device and keep its port id. > + * > + * When a port has to be reset passively, the DPDK application can > invoke > + * this function. For example when a PF is reset, all its VFs should > also > + * be reset. Normally a DPDK application can invoke this function > when > + * RTE_ETH_EVENT_INTR_RESET event is detected, but can also use it > to start > + * a port reset in other circumstances. > + * > + * When this function is called, it first stops the port and then > calls the > + * PMD specific dev_uninit( ) and dev_init( ) to return the port to > initial > + * state, in which no Tx and Rx queues are setup, as if the port has > been > + * reset and not started. The port keeps the port id it had before > the > + * function call. > + * > + * After calling rte_eth_dev_reset( ), the application should use > + * rte_eth_dev_configure( ), rte_eth_rx_queue_setup( ), > + * rte_eth_tx_queue_setup( ), and rte_eth_dev_start( ) > + * to reconfigure the device as appropriate. > + * > + * Note: To avoid unexpected behavior, the application should stop > calling > + * Tx and Rx functions before calling rte_eth_dev_reset( ). For > thread > + * safety, all these controlling functions should be called from the > same > + * thread. > + * > + * @param port_id > + *=C2=A0=C2=A0=C2=A0The port identifier of the Ethernet device. > + */ > +int rte_eth_dev_reset(uint8_t port_id); > + > +/** > =C2=A0 * Enable receipt in promiscuous mode for an Ethernet device. > =C2=A0 * > =C2=A0 * @param port_id > diff --git a/lib/librte_ether/rte_ether_version.map > b/lib/librte_ether/rte_ether_version.map > index 4283728..e86d51e 100644 > --- a/lib/librte_ether/rte_ether_version.map > +++ b/lib/librte_ether/rte_ether_version.map > @@ -155,6 +155,7 @@ DPDK_17.08 { > =C2=A0 rte_eth_dev_adjust_nb_rx_tx_desc; > =C2=A0 rte_flow_copy; > =C2=A0 rte_flow_isolate; > + rte_eth_dev_reset; > =C2=A0 rte_tm_capabilities_get; > =C2=A0 rte_tm_get_leaf_nodes; > =C2=A0 rte_tm_hierarchy_commit; Hello Wei, First of all thanks for your work, we have a use case where it's extremely important to be able to reset the VFs when the PF (Linux) changes state. Question: can the rte_eth_dev_reset call block in the current implementation that was merged? Last year a patchset was sent by, I believe, a colleague of yours, Wenzhuo Lu, and I've been using it in production. One of the things that I immediately noticed is that the _reset call was blocking - that was a show stopper for our application, so I sent a couple of patches to take a "blocking" boolean, and return EAGAIN instead of blocking. You can find the original patches this link: http://dpdk.org/ml/archives/dev/2016-June/042018.html The description of the problem we had with the blocking call: http://dpdk.org/ml/archives/dev/2016-July/043446.html The "blocking" and EAGAIN changes: http://dpdk.org/ml/archives/dev/2016-July/043535.html http://dpdk.org/ml/archives/dev/2016-July/043818.html Functionality-wise, from the point of view of an application, are you aware of big differences between that patchset and what was merged today? Thanks! --=20 Kind regards, Luca Boccassi