[dpdk-dev] [PATCH v4] ethdev: allow returning error on VLAN offload configuration

David Harton (dharton) dharton at cisco.com
Thu Sep 7 14:09:20 CEST 2017


> -----Original Message-----
> From: Hemant Agrawal [mailto:hemant.agrawal at nxp.com]
> 
> On 9/1/2017 6:24 PM, David Harton (dharton) wrote:
> >
> >> -----Original Message-----
> >> From: Hemant Agrawal [mailto:hemant.agrawal at 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 <dharton at cisco.com>
> >>> ---
> 
> <snip>....
> 
> >>> 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 = 0;
> >>>  	int mask = 0;
> >>> -	int cur, org = 0;
> >>> +	int cur, orig = 0;
> >>> +	uint8_t orig_strip, orig_filter, orig_extend;
> >>>
> >>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >>>  	dev = &rte_eth_devices[port_id];
> >>>
> >>> +	/* save original values in case of failure */
> >>> +	orig_strip = dev->data->dev_conf.rxmode.hw_vlan_strip;
> >>> +	orig_filter = dev->data->dev_conf.rxmode.hw_vlan_filter;
> >>> +	orig_extend = dev->data->dev_conf.rxmode.hw_vlan_extend;
> >>> +
> >>>  	/*check which option changed by application*/
> >>>  	cur = !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD);
> >>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> >>> -	if (cur != org) {
> >>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_strip);
> >>> +	if (cur != orig) {
> >>>  		dev->data->dev_conf.rxmode.hw_vlan_strip = (uint8_t)cur;
> >>>  		mask |= ETH_VLAN_STRIP_MASK;
> >>>  	}
> >>>
> >>>  	cur = !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD);
> >>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> >>> -	if (cur != org) {
> >>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_filter);
> >>> +	if (cur != orig) {
> >>>  		dev->data->dev_conf.rxmode.hw_vlan_filter = (uint8_t)cur;
> >>>  		mask |= ETH_VLAN_FILTER_MASK;
> >>>  	}
> >>>
> >>>  	cur = !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD);
> >>> -	org = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> >>> -	if (cur != org) {
> >>> +	orig = !!(dev->data->dev_conf.rxmode.hw_vlan_extend);
> >>> +	if (cur != orig) {
> >>>  		dev->data->dev_conf.rxmode.hw_vlan_extend = (uint8_t)cur;
> >>>  		mask |= 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 = (*dev->dev_ops->vlan_offload_set)(dev, mask);
> >>> +	if (ret) {
> >>> +		/* hit an error restore  original values */
> >>> +		dev->data->dev_conf.rxmode.hw_vlan_strip = orig_strip;
> >>> +		dev->data->dev_conf.rxmode.hw_vlan_filter = orig_filter;
> >>> +		dev->data->dev_conf.rxmode.hw_vlan_extend = 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.
> 
> I agree that solving this issue is outside the scope of this patch.
> 
> However this patch may leave the drivers in incorrect state.

I agree.  When failures happen in other APIs as well, whether reported 
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.

> 
> Earlier this API was not returning error or failing. With this change, the
> 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 original
> 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 
the system should, as much as possible, remain the same as prior to the 
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 
implementation did?
 
> 
> The change is fine w.r.t dpaa2 driver.

Thanks.  And thanks for the good discussion.


More information about the dev mailing list