[dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[Somnath Kotur]

When user reissues same flow director cmd with a different queue
update the existing filter to redirect flow to the new desired
queue as destination just like the other filters like 5 tuple and
generic flow.

Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_ethdev.c | 45 ++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 21c46f8..b47a2fb 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -2358,7 +2358,8 @@ bnxt_parse_fdir_filter(struct bnxt *bp,
 }
 
 static struct bnxt_filter_info *
-bnxt_match_fdir(struct bnxt *bp, struct bnxt_filter_info *nf)
+bnxt_match_fdir(struct bnxt *bp, struct bnxt_filter_info *nf,
+		struct bnxt_vnic_info **mvnic)
 {
 	struct bnxt_filter_info *mf = NULL;
 	int i;
@@ -2396,8 +2397,11 @@ bnxt_match_fdir(struct bnxt *bp, struct bnxt_filter_info *nf)
 			    !memcmp(mf->dst_ipaddr, nf->dst_ipaddr,
 				    sizeof(nf->dst_ipaddr)) &&
 			    !memcmp(mf->dst_ipaddr_mask, nf->dst_ipaddr_mask,
-				    sizeof(nf->dst_ipaddr_mask)))
+				    sizeof(nf->dst_ipaddr_mask))) {
+				if (mvnic)
+					*mvnic = vnic;
 				return mf;
+			}
 		}
 	}
 	return NULL;
@@ -2411,7 +2415,7 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
 	struct bnxt *bp = (struct bnxt *)dev->data->dev_private;
 	struct rte_eth_fdir_filter *fdir  = (struct rte_eth_fdir_filter *)arg;
 	struct bnxt_filter_info *filter, *match;
-	struct bnxt_vnic_info *vnic;
+	struct bnxt_vnic_info *vnic, *mvnic;
 	int ret = 0, i;
 
 	if (filter_op == RTE_ETH_FILTER_NOP)
@@ -2436,11 +2440,32 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
 			goto free_filter;
 		filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
 
-		match = bnxt_match_fdir(bp, filter);
+		if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
+			vnic = STAILQ_FIRST(&bp->ff_pool[0]);
+		else
+			vnic =
+			STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);
+
+		match = bnxt_match_fdir(bp, filter, &mvnic);
 		if (match != NULL && filter_op == RTE_ETH_FILTER_ADD) {
-			PMD_DRV_LOG(ERR, "Flow already exists.\n");
-			ret = -EEXIST;
-			goto free_filter;
+			if (match->dst_id == vnic->fw_vnic_id) {
+				PMD_DRV_LOG(ERR, "Flow already exists.\n");
+				ret = -EEXIST;
+				goto free_filter;
+			} else {
+				match->dst_id = vnic->fw_vnic_id;
+				ret = bnxt_hwrm_set_ntuple_filter(bp,
+								  match->dst_id,
+								  match);
+				STAILQ_REMOVE(&mvnic->filter, match,
+					      bnxt_filter_info, next);
+				STAILQ_INSERT_TAIL(&vnic->filter, match, next);
+				PMD_DRV_LOG(ERR,
+					"Filter with matching pattern exist\n");
+				PMD_DRV_LOG(ERR,
+					"Updated it to new destination q\n");
+				goto free_filter;
+			}
 		}
 		if (match == NULL && filter_op == RTE_ETH_FILTER_DELETE) {
 			PMD_DRV_LOG(ERR, "Flow does not exist.\n");
@@ -2448,12 +2473,6 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
 			goto free_filter;
 		}
 
-		if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
-			vnic = STAILQ_FIRST(&bp->ff_pool[0]);
-		else
-			vnic =
-			STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);
-
 		if (filter_op == RTE_ETH_FILTER_ADD) {
 			ret = bnxt_hwrm_set_ntuple_filter(bp,
 							  filter->dst_id,
-- 
2.10.1.613.g2cc2e70

Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[Ferruh Yigit]

On 2/22/2018 2:58 AM, Somnath Kotur wrote:

Please start with lowercase after "net/bnxt: fix"

> When user reissues same flow director cmd with a different queue
> update the existing filter to redirect flow to the new desired
> queue as destination just like the other filters like 5 tuple and
> generic flow.

Can you please add a fixes line?

Also ./devtools/check-git-log.sh complains about long title, what about
something like (just a sample):
"net/bnxt: fix flow director with same cmd different queue"

> 
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>

<...>

> @@ -2436,11 +2440,32 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
>  			goto free_filter;
>  		filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
>  
> -		match = bnxt_match_fdir(bp, filter);
> +		if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
> +			vnic = STAILQ_FIRST(&bp->ff_pool[0]);
> +		else
> +			vnic =
> +			STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);

Is this done because of column limit? If so I would prefer a few extra chars
instead of this assignment.

btw, not related to this patch, but in this switch there are a few "/*
FALLTHROUGH */" comments but they may not be required (or wrong), can you please
check.

<...>

Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[Somnath Kotur]

Hi Ferruh,


On Fri, Mar 23, 2018 at 12:16 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 2/22/2018 2:58 AM, Somnath Kotur wrote:
>
> Please start with lowercase after "net/bnxt: fix"
>
> > When user reissues same flow director cmd with a different queue
> > update the existing filter to redirect flow to the new desired
> > queue as destination just like the other filters like 5 tuple and
> > generic flow.
>
> Can you please add a fixes line?
>
Actually, I'm not sure if this qualifies as a 'fix for a regression', it
prescribes a new behavior for this scenario (of same flow-director cmd ,
different queue)  we never handled this before at all , Do you still think
it needs it ?

>
> Also ./devtools/check-git-log.sh complains about long title, what about
> something like (just a sample):
> "net/bnxt: fix flow director with same cmd different queue"
>
> >
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>
> <...>
>
> > @@ -2436,11 +2440,32 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
> >                       goto free_filter;
> >               filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
> >
> > -             match = bnxt_match_fdir(bp, filter);
> > +             if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
> > +                     vnic = STAILQ_FIRST(&bp->ff_pool[0]);
> > +             else
> > +                     vnic =
> > +                     STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);
>
> Is this done because of column limit? If so I would prefer a few extra
> chars
> instead of this assignment.
>
Yes, please thank you , it was going over by 1 character :)

>
> btw, not related to this patch, but in this switch there are a few "/*
> FALLTHROUGH */" comments but they may not be required (or wrong), can you
> please
> check.
>
> Sure , will do.

> <...>
>

Thanks
Som

Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[Ferruh Yigit]

On 3/23/2018 3:34 AM, Somnath Kotur wrote:
> Hi Ferruh,
> 
> 
> On Fri, Mar 23, 2018 at 12:16 AM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 2/22/2018 2:58 AM, Somnath Kotur wrote:
> 
>     Please start with lowercase after "net/bnxt: fix"
> 
>     > When user reissues same flow director cmd with a different queue
>     > update the existing filter to redirect flow to the new desired
>     > queue as destination just like the other filters like 5 tuple and
>     > generic flow.
> 
>     Can you please add a fixes line?
> 
> Actually, I'm not sure if this qualifies as a 'fix for a regression', it
> prescribes a new behavior for this scenario (of same flow-director cmd ,
> different queue)  we never handled this before at all , Do you still think it
> needs it ?

Patch title starts with "fix bug with ..." :)

Previously it was returning error when filter exist, now it is updating the
existing filter with new destination. If this is fix or not depends on expected
behavior [1].

Practical reasons of having fixes tag:
1- Stable trees checks for Fixes tag and gets patch to stable trees, if you want
you patch backported the tag is required.

2- To help other developers that wants to work on your code, these tags helps to
trace easier and understand code easier.

3- Helps reviewers understand the scope and help on prioritization.


If this is not fix please drop "fix" from patch title and be aware that this
won't be included into stable trees. If this is a fix please add the fixes line.


[1]
technically a code requires update:
- To add a new feature (justify a new expectation)
- To refactor (improve readability, improve re-usability etc.. but same
functionality)
- To fix the functionality to make it behave as expected.

It is hard to claim a fix without clear expectations / requirements. And I think
code updates triggered because of expectation changes fits into first category.

> 
> 
>     Also ./devtools/check-git-log.sh complains about long title, what about
>     something like (just a sample):
>     "net/bnxt: fix flow director with same cmd different queue"
> 
>     >
>     > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com
>     <mailto:somnath.kotur@broadcom.com>>
> 
>     <...>
> 
>     > @@ -2436,11 +2440,32 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
>     >                       goto free_filter;
>     >               filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
>     >
>     > -             match = bnxt_match_fdir(bp, filter);
>     > +             if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
>     > +                     vnic = STAILQ_FIRST(&bp->ff_pool[0]);
>     > +             else
>     > +                     vnic =
>     > +                     STAILQ_FIRST(&bp->ff_pool[fdir->action.rx_queue]);
> 
>     Is this done because of column limit? If so I would prefer a few extra chars
>     instead of this assignment.
> 
> Yes, please thank you , it was going over by 1 character :) 
> 
> 
>     btw, not related to this patch, but in this switch there are a few "/*
>     FALLTHROUGH */" comments but they may not be required (or wrong), can you please
>     check.
> 
> Sure , will do. 
> 
>     <...>
> 
> 
> Thanks
> Som

Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director

[Somnath Kotur]

On Fri, Mar 23, 2018 at 4:11 PM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 3/23/2018 3:34 AM, Somnath Kotur wrote:
> > Hi Ferruh,
> >
> >
> > On Fri, Mar 23, 2018 at 12:16 AM, Ferruh Yigit <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> >     On 2/22/2018 2:58 AM, Somnath Kotur wrote:
> >
> >     Please start with lowercase after "net/bnxt: fix"
> >
> >     > When user reissues same flow director cmd with a different queue
> >     > update the existing filter to redirect flow to the new desired
> >     > queue as destination just like the other filters like 5 tuple and
> >     > generic flow.
> >
> >     Can you please add a fixes line?
> >
> > Actually, I'm not sure if this qualifies as a 'fix for a regression', it
> > prescribes a new behavior for this scenario (of same flow-director cmd ,
> > different queue)  we never handled this before at all , Do you still
> think it
> > needs it ?
>
> Patch title starts with "fix bug with ..." :)
>
Yes i was aware of the irony,  while i was asking the question, was just
thinking /introspecting aloud :)
Thank you for the detailed explanation. All things considered, i think it's
better for me to right now go
with the 'fix' since it helps in so many ways that i hadn't considered ...
So will send out the respin soon with the 'fix' intact


>
> Previously it was returning error when filter exist, now it is updating the
> existing filter with new destination. If this is fix or not depends on
> expected
> behavior [1].
>
> Practical reasons of having fixes tag:
> 1- Stable trees checks for Fixes tag and gets patch to stable trees, if
> you want
> you patch backported the tag is required.
>
> 2- To help other developers that wants to work on your code, these tags
> helps to
> trace easier and understand code easier.
>
> 3- Helps reviewers understand the scope and help on prioritization.
>
>
> If this is not fix please drop "fix" from patch title and be aware that
> this
> won't be included into stable trees. If this is a fix please add the fixes
> line.
>
>
> [1]
> technically a code requires update:
> - To add a new feature (justify a new expectation)
> - To refactor (improve readability, improve re-usability etc.. but same
> functionality)
> - To fix the functionality to make it behave as expected.
>
> It is hard to claim a fix without clear expectations / requirements. And I
> think
> code updates triggered because of expectation changes fits into first
> category.
>
> >
> >
> >     Also ./devtools/check-git-log.sh complains about long title, what
> about
> >     something like (just a sample):
> >     "net/bnxt: fix flow director with same cmd different queue"
> >
> >     >
> >     > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com
> >     <mailto:somnath.kotur@broadcom.com>>
> >
> >     <...>
> >
> >     > @@ -2436,11 +2440,32 @@ bnxt_fdir_filter(struct rte_eth_dev *dev,
> >     >                       goto free_filter;
> >     >               filter->filter_type = HWRM_CFA_NTUPLE_FILTER;
> >     >
> >     > -             match = bnxt_match_fdir(bp, filter);
> >     > +             if (fdir->action.behavior == RTE_ETH_FDIR_REJECT)
> >     > +                     vnic = STAILQ_FIRST(&bp->ff_pool[0]);
> >     > +             else
> >     > +                     vnic =
> >     > +                     STAILQ_FIRST(&bp->ff_pool[
> fdir->action.rx_queue]);
> >
> >     Is this done because of column limit? If so I would prefer a few
> extra chars
> >     instead of this assignment.
> >
> > Yes, please thank you , it was going over by 1 character :)
> >
> >
> >     btw, not related to this patch, but in this switch there are a few
> "/*
> >     FALLTHROUGH */" comments but they may not be required (or wrong),
> can you please
> >     check.
> >
> > Sure , will do.
> >
> >     <...>
> >
> >
> > Thanks
> > Som
>
>