From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David Harton (dharton)" Subject: Re: [PATCH v4] ethdev: allow returning error on VLAN offload configuration Date: Thu, 7 Sep 2017 12:09:20 +0000 Message-ID: <3862084d353f4101a0b94110d869e390@XCH-RCD-016.cisco.com> References: <20170825134717.18376-1-dharton@cisco.com> <20170901023628.21308-1-dharton@cisco.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Hemant Agrawal , "thomas@monjalon.net" , "ferruh.yigit@intel.com" , "ajit.khaparde@broadcom.com" , "John Daley (johndale)" , "konstantin.ananyev@intel.com" , "jingjing.wu@intel.com" , "beilei.xing@intel.com" , "jing.d.chen@intel.com" , "adrien.mazarguil@6wind.com" , "nelio.laranjeiro@6wind.com" , "alejandro.lucero@netronome.com" , "rasesh.mody@cavium.com" , "harish.patil@cavium.com" , "skhare@vmware.com" , "yliu@fridaylinux.org" , "maxime.coquelin@redhat.com" Received: from alln-iport-6.cisco.com (alln-iport-6.cisco.com [173.37.142.93]) by dpdk.org (Postfix) with ESMTP id 30B97199A9 for ; Thu, 7 Sep 2017 14:09:23 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com] >=20 > On 9/1/2017 6:24 PM, David Harton (dharton) wrote: > > > >> -----Original Message----- > >> From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com] > >> > >> On 9/1/2017 8:06 AM, David Harton wrote: > >>> Some devices may not support or fail setting VLAN offload > >>> configuration based on dynamic circurmstances so the > >>> vlan_offload_set_t vector is modified to return an int so the caller > >>> can determine success or not. > >>> > >>> rte_eth_dev_set_vlan_offload is updated to return the value provided > >>> by the vector when called along with restoring the original offload > >>> configs on failure. > >>> > >>> Existing vlan_offload_set_t vectors are modified to return an int. > >>> Majority of cases return 0 but a few that actually can fail now > >>> return their failure codes. > >>> > >>> Finally, a vlan_offload_set_t vector is added to virtio to > >>> facilitate dynamically turning VLAN strip on or off. > >>> > >>> Signed-off-by: David Harton > >>> --- >=20 > .... >=20 > >>> diff --git a/lib/librte_ether/rte_ethdev.c > >> b/lib/librte_ether/rte_ethdev.c > >>> index 0597641..05e52b8 100644 > >>> --- a/lib/librte_ether/rte_ethdev.c > >>> +++ b/lib/librte_ether/rte_ethdev.c > >>> @@ -2048,29 +2048,35 @@ struct rte_eth_dev * > >>> struct rte_eth_dev *dev; > >>> int ret =3D 0; > >>> int mask =3D 0; > >>> - int cur, org =3D 0; > >>> + int cur, orig =3D 0; > >>> + uint8_t orig_strip, orig_filter, orig_extend; > >>> > >>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > >>> dev =3D &rte_eth_devices[port_id]; > >>> > >>> + /* save original values in case of failure */ > >>> + orig_strip =3D dev->data->dev_conf.rxmode.hw_vlan_strip; > >>> + orig_filter =3D dev->data->dev_conf.rxmode.hw_vlan_filter; > >>> + orig_extend =3D dev->data->dev_conf.rxmode.hw_vlan_extend; > >>> + > >>> /*check which option changed by application*/ > >>> cur =3D !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD); > >>> - org =3D !!(dev->data->dev_conf.rxmode.hw_vlan_strip); > >>> - if (cur !=3D org) { > >>> + orig =3D !!(dev->data->dev_conf.rxmode.hw_vlan_strip); > >>> + if (cur !=3D orig) { > >>> dev->data->dev_conf.rxmode.hw_vlan_strip =3D (uint8_t)cur; > >>> mask |=3D ETH_VLAN_STRIP_MASK; > >>> } > >>> > >>> cur =3D !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD); > >>> - org =3D !!(dev->data->dev_conf.rxmode.hw_vlan_filter); > >>> - if (cur !=3D org) { > >>> + orig =3D !!(dev->data->dev_conf.rxmode.hw_vlan_filter); > >>> + if (cur !=3D orig) { > >>> dev->data->dev_conf.rxmode.hw_vlan_filter =3D (uint8_t)cur; > >>> mask |=3D ETH_VLAN_FILTER_MASK; > >>> } > >>> > >>> cur =3D !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD); > >>> - org =3D !!(dev->data->dev_conf.rxmode.hw_vlan_extend); > >>> - if (cur !=3D org) { > >>> + orig =3D !!(dev->data->dev_conf.rxmode.hw_vlan_extend); > >>> + if (cur !=3D orig) { > >>> dev->data->dev_conf.rxmode.hw_vlan_extend =3D (uint8_t)cur; > >>> mask |=3D ETH_VLAN_EXTEND_MASK; > >>> } > >>> @@ -2080,7 +2086,13 @@ struct rte_eth_dev * > >>> return ret; > >>> > >>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); > >>> - (*dev->dev_ops->vlan_offload_set)(dev, mask); > >>> + ret =3D (*dev->dev_ops->vlan_offload_set)(dev, mask); > >>> + if (ret) { > >>> + /* hit an error restore original values */ > >>> + dev->data->dev_conf.rxmode.hw_vlan_strip =3D orig_strip; > >>> + dev->data->dev_conf.rxmode.hw_vlan_filter =3D orig_filter; > >>> + dev->data->dev_conf.rxmode.hw_vlan_extend =3D orig_extend; > >>> + } > >>> > >> Currently vlan offload is combining three functions: > >> strip, filter and extend. > >> > >> Not all PMDs in DPDK support all of three. > >> Should not the error we extended to reflect, which of the VLAN > >> offload is not supported? > > > > There are many examples of APIs that not all PMDs support. > > There are also other APIs that manipulate many attributes but do not > > communicate which attribute fails on set. > > Solving these issues I believe it outside the scope of this change and > > it requires a larger community discussion. > > > > This proposed change at least adheres to the existing paradigm. >=20 > I agree that solving this issue is outside the scope of this patch. >=20 > However this patch may leave the drivers in incorrect state. I agree. When failures happen in other APIs as well, whether reported=20 to the caller or not, how the failure is handled is inconsistent. Also, drivers could be in the incorrect state after this API call with the current implementation. >=20 > Earlier this API was not returning error or failing. With this change, th= e > driver may want to implement the error handling if 2nd or 3rd attribute > failed. Note that you are also assuming and reverting back to the origina= l > condition. The real thrust of this change is to report the failure so the caller can attempt to take some corrective action. Is there a policy one way or the other on whether DPDK restores original state on failures or just leaves the system in a failed state and reports the condition? I've always come from the ilk that if an API call fails then the state of=20 the system should, as much as possible, remain the same as prior to the=20 API call which is why I restored the flags on failure. With that, I can make ethdev call the device again with the "restored" values and still return the first failure. Or I could leave ethdev alone and leave the "bad state" as the previous=20 implementation did? =20 >=20 > The change is fine w.r.t dpaa2 driver. Thanks. And thanks for the good discussion.