[dpdk-dev] [PATCH v1] net/tap: fix isolation mode toggling

[dpdk-dev] [PATCH v1] net/tap: fix isolation mode toggling

[Ophir Munk]

Running testpmd command "flow isolae <port> 0" (i.e. disabling flow
isolation) followed by command "flow isolate <port> 1" (i.e. enabling
flow isolation) may result in a TAP error:
PMD: Kernel refused TC filter rule creation (17): File exists

Root cause analysis: when disabling flow isolation we keep the local
rule to redirect packets on TX (TAP_REMOTE_TX index) while we add it
again when enabling flow isolation. As a result this rule is added
two times in a raw which results in "File exists" error.
The fix is to identify the "File exists" error and silently ignore it.

Another issue occurs when enabling isolation mode several times in a
raw in which case the same tc rules are added consecutively and
rte_flow structs are added to a linked list before removing the
previous rte_flow structs.
The fix is to act upon isolation mode command only when there is a
change from "0" to "1" (or vice versa).

Fixes: f503d2694825 ("net/tap: support flow API isolated mode")
Cc: stable@dpdk.org

Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
 drivers/net/tap/tap_flow.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index aab9eef..91f15f6 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1568,10 +1568,10 @@ tap_flow_isolate(struct rte_eth_dev *dev,
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
 
-	if (set)
-		pmd->flow_isolate = 1;
-	else
-		pmd->flow_isolate = 0;
+	/* if already in the right isolation mode - nothing to do */
+	if ((!!set ^ pmd->flow_isolate) == 0)
+		return 0;
+	pmd->flow_isolate = !!set;
 	/*
 	 * If netdevice is there, setup appropriate flow rules immediately.
 	 * Otherwise it will be set when bringing up the netdevice (tun_alloc).
@@ -1579,21 +1579,30 @@ tap_flow_isolate(struct rte_eth_dev *dev,
 	if (!pmd->rxq[0].fd)
 		return 0;
 	if (set) {
-		struct rte_flow *flow;
+		struct rte_flow *remote_flow;
 
-		while (1) {
-			flow = LIST_FIRST(&pmd->implicit_flows);
-			if (!flow)
+		while (!LIST_EMPTY(&pmd->implicit_flows)) {
+			remote_flow = LIST_FIRST(&pmd->implicit_flows);
+			if (!remote_flow)
 				break;
 			/*
 			 * Remove all implicit rules on the remote.
 			 * Keep the local rule to redirect packets on TX.
 			 * Keep also the last implicit local rule: ISOLATE.
 			 */
-			if (flow->msg.t.tcm_ifindex == pmd->if_index)
-				break;
-			if (tap_flow_destroy_pmd(pmd, flow, NULL) < 0)
-				goto error;
+			if (remote_flow->msg.t.tcm_ifindex != pmd->if_index) {
+				/*
+				 * remove TC from kernel and
+				 * remote_flow from list
+				 */
+				if (tap_flow_destroy_pmd(pmd, remote_flow,
+						NULL) < 0)
+					goto error;
+			} else {
+				/* remove remote_flow from list */
+				LIST_REMOVE(remote_flow, next);
+				rte_free(remote_flow);
+			}
 		}
 		/* Switch the TC rule according to pmd->flow_isolate */
 		if (tap_flow_implicit_create(pmd, TAP_ISOLATE) == -1)
@@ -1739,8 +1748,8 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	}
 	err = tap_nl_recv_ack(pmd->nlsk_fd);
 	if (err < 0) {
-		/* Silently ignore re-entering remote promiscuous rule */
-		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
+		/* Silently ignore re-entering existing rule */
+		if (errno == EEXIST)
 			goto success;
 		TAP_LOG(ERR,
 			"Kernel refused TC filter rule creation (%d): %s",
-- 
2.7.4

Re: [dpdk-dev] [PATCH v1] net/tap: fix isolation mode toggling

[Wiles, Keith]

> On May 7, 2018, at 3:36 AM, Ophir Munk <ophirmu@mellanox.com> wrote:
> 
> Running testpmd command "flow isolae <port> 0" (i.e. disabling flow
> isolation) followed by command "flow isolate <port> 1" (i.e. enabling
> flow isolation) may result in a TAP error:
> PMD: Kernel refused TC filter rule creation (17): File exists
> 
> Root cause analysis: when disabling flow isolation we keep the local
> rule to redirect packets on TX (TAP_REMOTE_TX index) while we add it
> again when enabling flow isolation. As a result this rule is added
> two times in a raw which results in "File exists" error.

/raw/row
> The fix is to identify the "File exists" error and silently ignore it.
> 
> Another issue occurs when enabling isolation mode several times in a
> raw in which case the same tc rules are added consecutively and

/raw/row
> rte_flow structs are added to a linked list before removing the
> previous rte_flow structs.
> The fix is to act upon isolation mode command only when there is a
> change from "0" to "1" (or vice versa).
> 
> Fixes: f503d2694825 ("net/tap: support flow API isolated mode")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> drivers/net/tap/tap_flow.c | 37 +++++++++++++++++++++++--------------
> 1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index aab9eef..91f15f6 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -1568,10 +1568,10 @@ tap_flow_isolate(struct rte_eth_dev *dev,
> {
> 	struct pmd_internals *pmd = dev->data->dev_private;
> 
> -	if (set)
> -		pmd->flow_isolate = 1;
> -	else
> -		pmd->flow_isolate = 0;
> +	/* if already in the right isolation mode - nothing to do */
> +	if ((!!set ^ pmd->flow_isolate) == 0)
> +		return 0;
> +	pmd->flow_isolate = !!set;

Using double negation is not very readable IMO, I would prefer this converted to a true boolean type if required.

variable ‘set' here should a 0 or 1 already, please expand this code to not use !!, using this in modern compilers should not be required. I understand this maybe shorted to write, but not very readable IMO and we need to make DPDK readable.

> 	/*
> 	 * If netdevice is there, setup appropriate flow rules immediately.
> 	 * Otherwise it will be set when bringing up the netdevice (tun_alloc).
> @@ -1579,21 +1579,30 @@ tap_flow_isolate(struct rte_eth_dev *dev,
> 	if (!pmd->rxq[0].fd)
> 		return 0;
> 	if (set) {
> -		struct rte_flow *flow;
> +		struct rte_flow *remote_flow;
> 
> -		while (1) {
> -			flow = LIST_FIRST(&pmd->implicit_flows);
> -			if (!flow)
> +		while (!LIST_EMPTY(&pmd->implicit_flows)) {
> +			remote_flow = LIST_FIRST(&pmd->implicit_flows);
> +			if (!remote_flow)
> 				break;
> 			/*
> 			 * Remove all implicit rules on the remote.
> 			 * Keep the local rule to redirect packets on TX.
> 			 * Keep also the last implicit local rule: ISOLATE.
> 			 */
> -			if (flow->msg.t.tcm_ifindex == pmd->if_index)
> -				break;
> -			if (tap_flow_destroy_pmd(pmd, flow, NULL) < 0)
> -				goto error;
> +			if (remote_flow->msg.t.tcm_ifindex != pmd->if_index) {
> +				/*
> +				 * remove TC from kernel and
> +				 * remote_flow from list
> +				 */
> +				if (tap_flow_destroy_pmd(pmd, remote_flow,
> +						NULL) < 0)
> +					goto error;
> +			} else {
> +				/* remove remote_flow from list */
> +				LIST_REMOVE(remote_flow, next);
> +				rte_free(remote_flow);
> +			}
> 		}
> 		/* Switch the TC rule according to pmd->flow_isolate */
> 		if (tap_flow_implicit_create(pmd, TAP_ISOLATE) == -1)
> @@ -1739,8 +1748,8 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
> 	}
> 	err = tap_nl_recv_ack(pmd->nlsk_fd);
> 	if (err < 0) {
> -		/* Silently ignore re-entering remote promiscuous rule */
> -		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
> +		/* Silently ignore re-entering existing rule */
> +		if (errno == EEXIST)
> 			goto success;
> 		TAP_LOG(ERR,
> 			"Kernel refused TC filter rule creation (%d): %s",
> -- 
> 2.7.4
> 

Regards,
Keith

Re: [dpdk-dev] [PATCH v1] net/tap: fix isolation mode toggling

[Ophir Munk]

Hi Keith,

> -----Original Message-----
> From: Wiles, Keith [mailto:keith.wiles@intel.com]
> Sent: Monday, May 14, 2018 3:33 PM
> To: Ophir Munk <ophirmu@mellanox.com>
> Cc: dev@dpdk.org; Pascal Mazon <pascal.mazon@6wind.com>; Thomas
> Monjalon <thomas@monjalon.net>; Olga Shern <olgas@mellanox.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] net/tap: fix isolation mode toggling
> 
> 
> 
> > On May 7, 2018, at 3:36 AM, Ophir Munk <ophirmu@mellanox.com>
> wrote:
> >
> > Running testpmd command "flow isolae <port> 0" (i.e. disabling flow
> > isolation) followed by command "flow isolate <port> 1" (i.e. enabling
> > flow isolation) may result in a TAP error:
> > PMD: Kernel refused TC filter rule creation (17): File exists
> >
> > Root cause analysis: when disabling flow isolation we keep the local
> > rule to redirect packets on TX (TAP_REMOTE_TX index) while we add it
> > again when enabling flow isolation. As a result this rule is added two
> > times in a raw which results in "File exists" error.
> 
> /raw/row

Fixed in v3

> > The fix is to identify the "File exists" error and silently ignore it.
> >
> > Another issue occurs when enabling isolation mode several times in a
> > raw in which case the same tc rules are added consecutively and
> 
> /raw/row

Fixed in v3

> > rte_flow structs are added to a linked list before removing the
> > previous rte_flow structs.
> > The fix is to act upon isolation mode command only when there is a
> > change from "0" to "1" (or vice versa).
> >
> > Fixes: f503d2694825 ("net/tap: support flow API isolated mode")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> > drivers/net/tap/tap_flow.c | 37 +++++++++++++++++++++++--------------
> > 1 file changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> > index aab9eef..91f15f6 100644
> > --- a/drivers/net/tap/tap_flow.c
> > +++ b/drivers/net/tap/tap_flow.c
> > @@ -1568,10 +1568,10 @@ tap_flow_isolate(struct rte_eth_dev *dev, {
> > 	struct pmd_internals *pmd = dev->data->dev_private;
> >
> > -	if (set)
> > -		pmd->flow_isolate = 1;
> > -	else
> > -		pmd->flow_isolate = 0;
> > +	/* if already in the right isolation mode - nothing to do */
> > +	if ((!!set ^ pmd->flow_isolate) == 0)
> > +		return 0;
> > +	pmd->flow_isolate = !!set;
> 
> Using double negation is not very readable IMO, I would prefer this
> converted to a true boolean type if required.
> 

Double negation usage was eliminated. 

> variable ‘set' here should a 0 or 1 already, please expand this code to not use
> !!, using this in modern compilers should not be required. I understand this
> maybe shorted to write, but not very readable IMO and we need to make
> DPDK readable.
> 
> > 	/*
> > 	 * If netdevice is there, setup appropriate flow rules immediately.
> > 	 * Otherwise it will be set when bringing up the netdevice (tun_alloc).
> > @@ -1579,21 +1579,30 @@ tap_flow_isolate(struct rte_eth_dev *dev,
> > 	if (!pmd->rxq[0].fd)
> > 		return 0;
> > 	if (set) {
> > -		struct rte_flow *flow;
> > +		struct rte_flow *remote_flow;
> >
> > -		while (1) {
> > -			flow = LIST_FIRST(&pmd->implicit_flows);
> > -			if (!flow)
> > +		while (!LIST_EMPTY(&pmd->implicit_flows)) {
> > +			remote_flow = LIST_FIRST(&pmd->implicit_flows);
> > +			if (!remote_flow)
> > 				break;
> > 			/*
> > 			 * Remove all implicit rules on the remote.
> > 			 * Keep the local rule to redirect packets on TX.
> > 			 * Keep also the last implicit local rule: ISOLATE.
> > 			 */
> > -			if (flow->msg.t.tcm_ifindex == pmd->if_index)
> > -				break;
> > -			if (tap_flow_destroy_pmd(pmd, flow, NULL) < 0)
> > -				goto error;
> > +			if (remote_flow->msg.t.tcm_ifindex != pmd-
> >if_index) {
> > +				/*
> > +				 * remove TC from kernel and
> > +				 * remote_flow from list
> > +				 */
> > +				if (tap_flow_destroy_pmd(pmd,
> remote_flow,
> > +						NULL) < 0)
> > +					goto error;
> > +			} else {
> > +				/* remove remote_flow from list */
> > +				LIST_REMOVE(remote_flow, next);
> > +				rte_free(remote_flow);
> > +			}
> > 		}
> > 		/* Switch the TC rule according to pmd->flow_isolate */
> > 		if (tap_flow_implicit_create(pmd, TAP_ISOLATE) == -1) @@ -
> 1739,8
> > +1748,8 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
> > 	}
> > 	err = tap_nl_recv_ack(pmd->nlsk_fd);
> > 	if (err < 0) {
> > -		/* Silently ignore re-entering remote promiscuous rule */
> > -		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
> > +		/* Silently ignore re-entering existing rule */
> > +		if (errno == EEXIST)
> > 			goto success;
> > 		TAP_LOG(ERR,
> > 			"Kernel refused TC filter rule creation (%d): %s",
> > --
> > 2.7.4
> >
> 
> Regards,
> Keith

Re: [dpdk-dev] [PATCH v1] net/tap: fix isolation mode toggling

[Wiles, Keith]

>> 
>> Using double negation is not very readable IMO, I would prefer this
>> converted to a true boolean type if required.
>> 
> 
> Double negation usage was eliminated. 

Thanks I acked already.

> 
>> variable ‘set' here should a 0 or 1 already, please expand this code to not use
>> !!, using this in modern compilers should not be required. I understand this
>> maybe shorted to write, but not very readable IMO and we need to make
>> DPDK readable.
>> 

Regards,
Keith

[dpdk-dev] [PATCH v2] net/tap: fix isolation mode toggling

[Ophir Munk]

Running testpmd command "flow isolae <port> 0" (i.e. disabling flow
isolation) followed by command "flow isolate <port> 1" (i.e. enabling
flow isolation) may result in a TAP error:
PMD: Kernel refused TC filter rule creation (17): File exists

Root cause analysis: when disabling flow isolation we keep the local
rule to redirect packets on TX (TAP_REMOTE_TX index) while we add it
again when enabling flow isolation. As a result this rule is added
two times in a row which results in "File exists" error.
The fix is to identify the "File exists" error and silently ignore it.

Another issue occurs when enabling isolation mode several times in a
raw in which case the same tc rules are added consecutively and
rte_flow structs are added to a linked list before removing the
previous rte_flow structs.
The fix is to act upon isolation mode command only when there is a
change from "0" to "1" (or vice versa).

Fixes: f503d2694825 ("net/tap: support flow API isolated mode")
Cc: stable@dpdk.org

Reviewed-by: Raslan Darawsheh <rasland@mellanox.com>
Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1:
initial release
v2:
1. Updates based on Keith Wiles review
2. Do not empty list of implicit TC rules (role back to legacy implementation)
   to ensure TC implicit rules cleanup during implicit rules flushing

 drivers/net/tap/tap_flow.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index aab9eef..6b60e6d 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1568,10 +1568,14 @@ tap_flow_isolate(struct rte_eth_dev *dev,
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
 
+	/* normalize 'set' variable to contain 0 or 1 values */
 	if (set)
-		pmd->flow_isolate = 1;
-	else
-		pmd->flow_isolate = 0;
+		set = 1;
+	/* if already in the right isolation mode - nothing to do */
+	if ((set ^ pmd->flow_isolate) == 0)
+		return 0;
+	/* mark the isolation mode for tap_flow_implicit_create() */
+	pmd->flow_isolate = set;
 	/*
 	 * If netdevice is there, setup appropriate flow rules immediately.
 	 * Otherwise it will be set when bringing up the netdevice (tun_alloc).
@@ -1579,20 +1583,20 @@ tap_flow_isolate(struct rte_eth_dev *dev,
 	if (!pmd->rxq[0].fd)
 		return 0;
 	if (set) {
-		struct rte_flow *flow;
+		struct rte_flow *remote_flow;
 
 		while (1) {
-			flow = LIST_FIRST(&pmd->implicit_flows);
-			if (!flow)
+			remote_flow = LIST_FIRST(&pmd->implicit_flows);
+			if (!remote_flow)
 				break;
 			/*
 			 * Remove all implicit rules on the remote.
 			 * Keep the local rule to redirect packets on TX.
 			 * Keep also the last implicit local rule: ISOLATE.
 			 */
-			if (flow->msg.t.tcm_ifindex == pmd->if_index)
+			if (remote_flow->msg.t.tcm_ifindex == pmd->if_index)
 				break;
-			if (tap_flow_destroy_pmd(pmd, flow, NULL) < 0)
+			if (tap_flow_destroy_pmd(pmd, remote_flow, NULL) < 0)
 				goto error;
 		}
 		/* Switch the TC rule according to pmd->flow_isolate */
@@ -1739,8 +1743,8 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	}
 	err = tap_nl_recv_ack(pmd->nlsk_fd);
 	if (err < 0) {
-		/* Silently ignore re-entering remote promiscuous rule */
-		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
+		/* Silently ignore re-entering existing rule */
+		if (errno == EEXIST)
 			goto success;
 		TAP_LOG(ERR,
 			"Kernel refused TC filter rule creation (%d): %s",
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] net/tap: fix isolation mode toggling

[Wiles, Keith]

> On May 14, 2018, at 5:11 PM, Ophir Munk <ophirmu@mellanox.com> wrote:
> 
> Running testpmd command "flow isolae <port> 0" (i.e. disabling flow
> isolation) followed by command "flow isolate <port> 1" (i.e. enabling
> flow isolation) may result in a TAP error:
> PMD: Kernel refused TC filter rule creation (17): File exists
> 
> Root cause analysis: when disabling flow isolation we keep the local
> rule to redirect packets on TX (TAP_REMOTE_TX index) while we add it
> again when enabling flow isolation. As a result this rule is added
> two times in a row which results in "File exists" error.
> The fix is to identify the "File exists" error and silently ignore it.
> 
> Another issue occurs when enabling isolation mode several times in a
> raw in which case the same tc rules are added consecutively and
> rte_flow structs are added to a linked list before removing the
> previous rte_flow structs.
> The fix is to act upon isolation mode command only when there is a
> change from "0" to "1" (or vice versa).
> 
> Fixes: f503d2694825 ("net/tap: support flow API isolated mode")
> Cc: stable@dpdk.org
> 
> Reviewed-by: Raslan Darawsheh <rasland@mellanox.com>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> ---
> v1:
> initial release
> v2:
> 1. Updates based on Keith Wiles review
> 2. Do not empty list of implicit TC rules (role back to legacy implementation)
>   to ensure TC implicit rules cleanup during implicit rules flushing
> 
> drivers/net/tap/tap_flow.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> index aab9eef..6b60e6d 100644
> --- a/drivers/net/tap/tap_flow.c
> +++ b/drivers/net/tap/tap_flow.c
> @@ -1568,10 +1568,14 @@ tap_flow_isolate(struct rte_eth_dev *dev,
> {
> 	struct pmd_internals *pmd = dev->data->dev_private;
> 
> +	/* normalize 'set' variable to contain 0 or 1 values */
> 	if (set)
> -		pmd->flow_isolate = 1;
> -	else
> -		pmd->flow_isolate = 0;
> +		set = 1;
> +	/* if already in the right isolation mode - nothing to do */
> +	if ((set ^ pmd->flow_isolate) == 0)
> +		return 0;
> +	/* mark the isolation mode for tap_flow_implicit_create() */
> +	pmd->flow_isolate = set;
> 	/*
> 	 * If netdevice is there, setup appropriate flow rules immediately.
> 	 * Otherwise it will be set when bringing up the netdevice (tun_alloc).
> @@ -1579,20 +1583,20 @@ tap_flow_isolate(struct rte_eth_dev *dev,
> 	if (!pmd->rxq[0].fd)
> 		return 0;
> 	if (set) {
> -		struct rte_flow *flow;
> +		struct rte_flow *remote_flow;
> 
> 		while (1) {
> -			flow = LIST_FIRST(&pmd->implicit_flows);
> -			if (!flow)
> +			remote_flow = LIST_FIRST(&pmd->implicit_flows);
> +			if (!remote_flow)
> 				break;
> 			/*
> 			 * Remove all implicit rules on the remote.
> 			 * Keep the local rule to redirect packets on TX.
> 			 * Keep also the last implicit local rule: ISOLATE.
> 			 */
> -			if (flow->msg.t.tcm_ifindex == pmd->if_index)
> +			if (remote_flow->msg.t.tcm_ifindex == pmd->if_index)
> 				break;
> -			if (tap_flow_destroy_pmd(pmd, flow, NULL) < 0)
> +			if (tap_flow_destroy_pmd(pmd, remote_flow, NULL) < 0)
> 				goto error;
> 		}
> 		/* Switch the TC rule according to pmd->flow_isolate */
> @@ -1739,8 +1743,8 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
> 	}
> 	err = tap_nl_recv_ack(pmd->nlsk_fd);
> 	if (err < 0) {
> -		/* Silently ignore re-entering remote promiscuous rule */
> -		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
> +		/* Silently ignore re-entering existing rule */
> +		if (errno == EEXIST)
> 			goto success;
> 		TAP_LOG(ERR,
> 			"Kernel refused TC filter rule creation (%d): %s",
> -- 
> 2.7.4
> 

Looks good.

Acked By: Keith Wiles<keith.wiles@intel.com>

Regards,
Keith

Re: [dpdk-dev] [PATCH v2] net/tap: fix isolation mode toggling

[Ophir Munk]

v2 has a typo (/raw/row/)
Re-sending as v3.

> -----Original Message-----
> From: Wiles, Keith [mailto:keith.wiles@intel.com]
> Sent: Tuesday, May 15, 2018 1:18 AM
> To: Ophir Munk <ophirmu@mellanox.com>
> Cc: DPDK <dev@dpdk.org>; Pascal Mazon <pascal.mazon@6wind.com>;
> Thomas Monjalon <thomas@monjalon.net>; Olga Shern
> <olgas@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>;
> stable@dpdk.org
> Subject: Re: [PATCH v2] net/tap: fix isolation mode toggling
> 
> 
> 
> > On May 14, 2018, at 5:11 PM, Ophir Munk <ophirmu@mellanox.com>
> wrote:
> >
> > Running testpmd command "flow isolae <port> 0" (i.e. disabling flow
> > isolation) followed by command "flow isolate <port> 1" (i.e. enabling
> > flow isolation) may result in a TAP error:
> > PMD: Kernel refused TC filter rule creation (17): File exists
> >
> > Root cause analysis: when disabling flow isolation we keep the local
> > rule to redirect packets on TX (TAP_REMOTE_TX index) while we add it
> > again when enabling flow isolation. As a result this rule is added two
> > times in a row which results in "File exists" error.
> > The fix is to identify the "File exists" error and silently ignore it.
> >
> > Another issue occurs when enabling isolation mode several times in a
> > raw in which case the same tc rules are added consecutively and
> > rte_flow structs are added to a linked list before removing the
> > previous rte_flow structs.
> > The fix is to act upon isolation mode command only when there is a
> > change from "0" to "1" (or vice versa).
> >
> > Fixes: f503d2694825 ("net/tap: support flow API isolated mode")
> > Cc: stable@dpdk.org
> >
> > Reviewed-by: Raslan Darawsheh <rasland@mellanox.com>
> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> > ---
> > v1:
> > initial release
> > v2:
> > 1. Updates based on Keith Wiles review 2. Do not empty list of
> > implicit TC rules (role back to legacy implementation)
> >   to ensure TC implicit rules cleanup during implicit rules flushing
> >
> > drivers/net/tap/tap_flow.c | 24 ++++++++++++++----------
> > 1 file changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
> > index aab9eef..6b60e6d 100644
> > --- a/drivers/net/tap/tap_flow.c
> > +++ b/drivers/net/tap/tap_flow.c
> > @@ -1568,10 +1568,14 @@ tap_flow_isolate(struct rte_eth_dev *dev, {
> > 	struct pmd_internals *pmd = dev->data->dev_private;
> >
> > +	/* normalize 'set' variable to contain 0 or 1 values */
> > 	if (set)
> > -		pmd->flow_isolate = 1;
> > -	else
> > -		pmd->flow_isolate = 0;
> > +		set = 1;
> > +	/* if already in the right isolation mode - nothing to do */
> > +	if ((set ^ pmd->flow_isolate) == 0)
> > +		return 0;
> > +	/* mark the isolation mode for tap_flow_implicit_create() */
> > +	pmd->flow_isolate = set;
> > 	/*
> > 	 * If netdevice is there, setup appropriate flow rules immediately.
> > 	 * Otherwise it will be set when bringing up the netdevice (tun_alloc).
> > @@ -1579,20 +1583,20 @@ tap_flow_isolate(struct rte_eth_dev *dev,
> > 	if (!pmd->rxq[0].fd)
> > 		return 0;
> > 	if (set) {
> > -		struct rte_flow *flow;
> > +		struct rte_flow *remote_flow;
> >
> > 		while (1) {
> > -			flow = LIST_FIRST(&pmd->implicit_flows);
> > -			if (!flow)
> > +			remote_flow = LIST_FIRST(&pmd->implicit_flows);
> > +			if (!remote_flow)
> > 				break;
> > 			/*
> > 			 * Remove all implicit rules on the remote.
> > 			 * Keep the local rule to redirect packets on TX.
> > 			 * Keep also the last implicit local rule: ISOLATE.
> > 			 */
> > -			if (flow->msg.t.tcm_ifindex == pmd->if_index)
> > +			if (remote_flow->msg.t.tcm_ifindex == pmd-
> >if_index)
> > 				break;
> > -			if (tap_flow_destroy_pmd(pmd, flow, NULL) < 0)
> > +			if (tap_flow_destroy_pmd(pmd, remote_flow, NULL)
> < 0)
> > 				goto error;
> > 		}
> > 		/* Switch the TC rule according to pmd->flow_isolate */ @@
> -1739,8
> > +1743,8 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
> > 	}
> > 	err = tap_nl_recv_ack(pmd->nlsk_fd);
> > 	if (err < 0) {
> > -		/* Silently ignore re-entering remote promiscuous rule */
> > -		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
> > +		/* Silently ignore re-entering existing rule */
> > +		if (errno == EEXIST)
> > 			goto success;
> > 		TAP_LOG(ERR,
> > 			"Kernel refused TC filter rule creation (%d): %s",
> > --
> > 2.7.4
> >
> 
> Looks good.
> 
> Acked By: Keith Wiles<keith.wiles@intel.com>
> 
> Regards,
> Keith

[dpdk-dev] [PATCH v3] net/tap: fix isolation mode toggling

[Ophir Munk]

Running testpmd command "flow isolae <port> 0" (i.e. disabling flow
isolation) followed by command "flow isolate <port> 1" (i.e. enabling
flow isolation) may result in a TAP error:
PMD: Kernel refused TC filter rule creation (17): File exists

Root cause analysis: when disabling flow isolation we keep the local
rule to redirect packets on TX (TAP_REMOTE_TX index) while we add it
again when enabling flow isolation. As a result this rule is added
two times in a row which results in "File exists" error.
The fix is to identify the "File exists" error and silently ignore it.

Another issue occurs when enabling isolation mode several times in a
row in which case the same tc rules are added consecutively and
rte_flow structs are added to a linked list before removing the
previous rte_flow structs.
The fix is to act upon isolation mode command only when there is a
change from "0" to "1" (or vice versa).

Fixes: f503d2694825 ("net/tap: support flow API isolated mode")
Cc: stable@dpdk.org

Reviewed-by: Raslan Darawsheh <rasland@mellanox.com>
Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
---
v1:
initial release
v2:
Please ignore (due to typo errors)
v3:
1. Updates based on Keith Wiles review
2. Do not empty list of implicit TC rules (role back to legacy implementation)
   to ensure TC implicit rules cleanup during implicit rules flushing

 drivers/net/tap/tap_flow.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/tap/tap_flow.c b/drivers/net/tap/tap_flow.c
index aab9eef..6b60e6d 100644
--- a/drivers/net/tap/tap_flow.c
+++ b/drivers/net/tap/tap_flow.c
@@ -1568,10 +1568,14 @@ tap_flow_isolate(struct rte_eth_dev *dev,
 {
 	struct pmd_internals *pmd = dev->data->dev_private;
 
+	/* normalize 'set' variable to contain 0 or 1 values */
 	if (set)
-		pmd->flow_isolate = 1;
-	else
-		pmd->flow_isolate = 0;
+		set = 1;
+	/* if already in the right isolation mode - nothing to do */
+	if ((set ^ pmd->flow_isolate) == 0)
+		return 0;
+	/* mark the isolation mode for tap_flow_implicit_create() */
+	pmd->flow_isolate = set;
 	/*
 	 * If netdevice is there, setup appropriate flow rules immediately.
 	 * Otherwise it will be set when bringing up the netdevice (tun_alloc).
@@ -1579,20 +1583,20 @@ tap_flow_isolate(struct rte_eth_dev *dev,
 	if (!pmd->rxq[0].fd)
 		return 0;
 	if (set) {
-		struct rte_flow *flow;
+		struct rte_flow *remote_flow;
 
 		while (1) {
-			flow = LIST_FIRST(&pmd->implicit_flows);
-			if (!flow)
+			remote_flow = LIST_FIRST(&pmd->implicit_flows);
+			if (!remote_flow)
 				break;
 			/*
 			 * Remove all implicit rules on the remote.
 			 * Keep the local rule to redirect packets on TX.
 			 * Keep also the last implicit local rule: ISOLATE.
 			 */
-			if (flow->msg.t.tcm_ifindex == pmd->if_index)
+			if (remote_flow->msg.t.tcm_ifindex == pmd->if_index)
 				break;
-			if (tap_flow_destroy_pmd(pmd, flow, NULL) < 0)
+			if (tap_flow_destroy_pmd(pmd, remote_flow, NULL) < 0)
 				goto error;
 		}
 		/* Switch the TC rule according to pmd->flow_isolate */
@@ -1739,8 +1743,8 @@ int tap_flow_implicit_create(struct pmd_internals *pmd,
 	}
 	err = tap_nl_recv_ack(pmd->nlsk_fd);
 	if (err < 0) {
-		/* Silently ignore re-entering remote promiscuous rule */
-		if (errno == EEXIST && idx == TAP_REMOTE_PROMISC)
+		/* Silently ignore re-entering existing rule */
+		if (errno == EEXIST)
 			goto success;
 		TAP_LOG(ERR,
 			"Kernel refused TC filter rule creation (%d): %s",
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3] net/tap: fix isolation mode toggling

[Wiles, Keith]

> On May 14, 2018, at 5:26 PM, Ophir Munk <ophirmu@mellanox.com> wrote:
> 
> Running testpmd command "flow isolae <port> 0" (i.e. disabling flow
> isolation) followed by command "flow isolate <port> 1" (i.e. enabling
> flow isolation) may result in a TAP error:
> PMD: Kernel refused TC filter rule creation (17): File exists
> 
> Root cause analysis: when disabling flow isolation we keep the local
> rule to redirect packets on TX (TAP_REMOTE_TX index) while we add it
> again when enabling flow isolation. As a result this rule is added
> two times in a row which results in "File exists" error.
> The fix is to identify the "File exists" error and silently ignore it.
> 
> Another issue occurs when enabling isolation mode several times in a
> row in which case the same tc rules are added consecutively and
> rte_flow structs are added to a linked list before removing the
> previous rte_flow structs.
> The fix is to act upon isolation mode command only when there is a
> change from "0" to "1" (or vice versa).
> 
> Fixes: f503d2694825 ("net/tap: support flow API isolated mode")
> Cc: stable@dpdk.org
> 
> Reviewed-by: Raslan Darawsheh <rasland@mellanox.com>
> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
> —

Acked by: Keith Wiles<keith.wile@intel.com>

Regards,
Keith

Re: [dpdk-dev] [dpdk-stable] [PATCH v3] net/tap: fix isolation mode toggling

[Ferruh Yigit]

On 5/14/2018 11:46 PM, Wiles, Keith wrote:
> 
> 
>> On May 14, 2018, at 5:26 PM, Ophir Munk <ophirmu@mellanox.com> wrote:
>>
>> Running testpmd command "flow isolae <port> 0" (i.e. disabling flow
>> isolation) followed by command "flow isolate <port> 1" (i.e. enabling
>> flow isolation) may result in a TAP error:
>> PMD: Kernel refused TC filter rule creation (17): File exists
>>
>> Root cause analysis: when disabling flow isolation we keep the local
>> rule to redirect packets on TX (TAP_REMOTE_TX index) while we add it
>> again when enabling flow isolation. As a result this rule is added
>> two times in a row which results in "File exists" error.
>> The fix is to identify the "File exists" error and silently ignore it.
>>
>> Another issue occurs when enabling isolation mode several times in a
>> row in which case the same tc rules are added consecutively and
>> rte_flow structs are added to a linked list before removing the
>> previous rte_flow structs.
>> The fix is to act upon isolation mode command only when there is a
>> change from "0" to "1" (or vice versa).
>>
>> Fixes: f503d2694825 ("net/tap: support flow API isolated mode")
>> Cc: stable@dpdk.org
>>
>> Reviewed-by: Raslan Darawsheh <rasland@mellanox.com>
>> Signed-off-by: Ophir Munk <ophirmu@mellanox.com>
>> —
> 
> Acked by: Keith Wiles<keith.wile@intel.com>

Applied to dpdk-next-net/master, thanks.