Re: [PATCH v4] ethdev: allow returning error on VLAN offload configuration

-----Original Message-----
From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com]

On 9/1/2017 6:24 PM, David Harton (dharton) wrote:

quoted

quoted

-----Original Message-----
From: Hemant Agrawal [mailto:hemant.agrawal@nxp.com]

On 9/1/2017 8:06 AM, David Harton wrote:

quoted

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 <redacted>
---

<snip>....

quoted

quoted

quoted

diff --git a/lib/librte_ether/rte_ethdev.c

b/lib/librte_ether/rte_ethdev.c

quoted

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.