diff mbox series

net/bnxt: allow the mark to use a cfa code of zero

Message ID 1590191701-8420-1-git-send-email-michael.baucom@broadcom.com (mailing list archive)
State Accepted, archived
Delegated to: Ajit Khaparde
Headers
Series net/bnxt: allow the mark to use a cfa code of zero |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues
ci/travis-robot success Travis build: passed

Commit Message

Mike Baucom May 22, 2020, 11:55 p.m. UTC 
  The mark code was too restrictive by disallowing a cfa_code of zero.
This code loosens the requirement and allows zero.

Fixes: b87abb2e55cb ("net/bnxt: support marking packet")

Signed-off-by: Mike Baucom <michael.baucom@broadcom.com>
Reviewed-by: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
---
 drivers/net/bnxt/bnxt_rxr.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Comments

Ajit Khaparde May 23, 2020, 3:52 a.m. UTC | #1 
On Fri, May 22, 2020 at 4:55 PM Mike Baucom <michael.baucom@broadcom.com>
wrote:

> The mark code was too restrictive by disallowing a cfa_code of zero.
> This code loosens the requirement and allows zero.
>
> Fixes: b87abb2e55cb ("net/bnxt: support marking packet")
>
> Signed-off-by: Mike Baucom <michael.baucom@broadcom.com>
> Reviewed-by: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
>
Applied to dpdk-next-net-brcm with updated commit headline [1].

[1] net/bnxt: fix mark action

> ---
>  drivers/net/bnxt/bnxt_rxr.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_rxr.c b/drivers/net/bnxt/bnxt_rxr.c
> index ee1acb1..91ff729 100644
> --- a/drivers/net/bnxt/bnxt_rxr.c
> +++ b/drivers/net/bnxt/bnxt_rxr.c
> @@ -465,17 +465,15 @@ static inline struct rte_mbuf *bnxt_tpa_end(
>                 break;
>         }
>
> -       if (cfa_code) {
> -               rc = ulp_mark_db_mark_get(bp->ulp_ctx, gfid,
> -                                         cfa_code, &mark_id);
> -               if (!rc) {
> -                       /* Got the mark, write it to the mbuf and return */
> -                       mbuf->hash.fdir.hi = mark_id;
> -                       mbuf->udata64 = (cfa_code & 0xffffffffull) << 32;
> -                       mbuf->hash.fdir.id = rxcmp1->cfa_code;
> -                       mbuf->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
> -                       return;
> -               }
> +       rc = ulp_mark_db_mark_get(bp->ulp_ctx, gfid,
> +                                 cfa_code, &mark_id);
> +       if (!rc) {
> +               /* Got the mark, write it to the mbuf and return */
> +               mbuf->hash.fdir.hi = mark_id;
> +               mbuf->udata64 = (cfa_code & 0xffffffffull) << 32;
> +               mbuf->hash.fdir.id = rxcmp1->cfa_code;
> +               mbuf->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
> +               return;
>         }
>
>  skip_mark:
> --
> 1.9.1
>
>
Ferruh Yigit May 27, 2020, 3:46 p.m. UTC | #2 
On 5/23/2020 12:55 AM, Mike Baucom wrote:
> The mark code was too restrictive by disallowing a cfa_code of zero.
> This code loosens the requirement and allows zero.

I can see "if (cfa_code)" check removed, but can you please give some details
that what it the impact of having that check and why change has been done.

Also it is not clear what "cfa code" is, if it has a significance can you please
describe it as well.

> 
> Fixes: b87abb2e55cb ("net/bnxt: support marking packet")
> 
> Signed-off-by: Mike Baucom <michael.baucom@broadcom.com>
> Reviewed-by: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
> ---
>  drivers/net/bnxt/bnxt_rxr.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/bnxt/bnxt_rxr.c b/drivers/net/bnxt/bnxt_rxr.c
> index ee1acb1..91ff729 100644
> --- a/drivers/net/bnxt/bnxt_rxr.c
> +++ b/drivers/net/bnxt/bnxt_rxr.c
> @@ -465,17 +465,15 @@ static inline struct rte_mbuf *bnxt_tpa_end(
>  		break;
>  	}
>  
> -	if (cfa_code) {
> -		rc = ulp_mark_db_mark_get(bp->ulp_ctx, gfid,
> -					  cfa_code, &mark_id);
> -		if (!rc) {
> -			/* Got the mark, write it to the mbuf and return */
> -			mbuf->hash.fdir.hi = mark_id;
> -			mbuf->udata64 = (cfa_code & 0xffffffffull) << 32;
> -			mbuf->hash.fdir.id = rxcmp1->cfa_code;
> -			mbuf->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
> -			return;
> -		}
> +	rc = ulp_mark_db_mark_get(bp->ulp_ctx, gfid,
> +				  cfa_code, &mark_id);
> +	if (!rc) {
> +		/* Got the mark, write it to the mbuf and return */
> +		mbuf->hash.fdir.hi = mark_id;
> +		mbuf->udata64 = (cfa_code & 0xffffffffull) << 32;
> +		mbuf->hash.fdir.id = rxcmp1->cfa_code;
> +		mbuf->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
> +		return;
>  	}
>  
>  skip_mark:
>
Ajit Khaparde May 27, 2020, 8:28 p.m. UTC | #3 
On Wed, May 27, 2020 at 8:47 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 5/23/2020 12:55 AM, Mike Baucom wrote:
> > The mark code was too restrictive by disallowing a cfa_code of zero.
> > This code loosens the requirement and allows zero.
>
> I can see "if (cfa_code)" check removed, but can you please give some
> details
> that what it the impact of having that check and why change has been done.
>
> Also it is not clear what "cfa code" is, if it has a significance can you
> please
> describe it as well.
>
In the ingress path, the cfa_code field in Rx completion identifies the
CFA action rule that was used for the incoming packet. It is possible
that the packet could hit the rule at index 0 in the table.

The mark action code was too restrictive by disallowing a cfa_code of zero.
This code in the patch loosens the requirement and allows zero.

I will update the commit log with the same verbiage.

Thanks
Ajit


>
> >
> > Fixes: b87abb2e55cb ("net/bnxt: support marking packet")
> >
> > Signed-off-by: Mike Baucom <michael.baucom@broadcom.com>
> > Reviewed-by: Kishore Padmanabha <kishore.padmanabha@broadcom.com>
> > ---
> >  drivers/net/bnxt/bnxt_rxr.c | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/bnxt/bnxt_rxr.c b/drivers/net/bnxt/bnxt_rxr.c
> > index ee1acb1..91ff729 100644
> > --- a/drivers/net/bnxt/bnxt_rxr.c
> > +++ b/drivers/net/bnxt/bnxt_rxr.c
> > @@ -465,17 +465,15 @@ static inline struct rte_mbuf *bnxt_tpa_end(
> >               break;
> >       }
> >
> > -     if (cfa_code) {
> > -             rc = ulp_mark_db_mark_get(bp->ulp_ctx, gfid,
> > -                                       cfa_code, &mark_id);
> > -             if (!rc) {
> > -                     /* Got the mark, write it to the mbuf and return */
> > -                     mbuf->hash.fdir.hi = mark_id;
> > -                     mbuf->udata64 = (cfa_code & 0xffffffffull) << 32;
> > -                     mbuf->hash.fdir.id = rxcmp1->cfa_code;
> > -                     mbuf->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
> > -                     return;
> > -             }
> > +     rc = ulp_mark_db_mark_get(bp->ulp_ctx, gfid,
> > +                               cfa_code, &mark_id);
> > +     if (!rc) {
> > +             /* Got the mark, write it to the mbuf and return */
> > +             mbuf->hash.fdir.hi = mark_id;
> > +             mbuf->udata64 = (cfa_code & 0xffffffffull) << 32;
> > +             mbuf->hash.fdir.id = rxcmp1->cfa_code;
> > +             mbuf->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
> > +             return;
> >       }
> >
> >  skip_mark:
> >
>
>
diff mbox series

Patch

diff --git a/drivers/net/bnxt/bnxt_rxr.c b/drivers/net/bnxt/bnxt_rxr.c
index ee1acb1..91ff729 100644
--- a/drivers/net/bnxt/bnxt_rxr.c
+++ b/drivers/net/bnxt/bnxt_rxr.c
@@ -465,17 +465,15 @@  static inline struct rte_mbuf *bnxt_tpa_end(
 		break;
 	}
 
-	if (cfa_code) {
-		rc = ulp_mark_db_mark_get(bp->ulp_ctx, gfid,
-					  cfa_code, &mark_id);
-		if (!rc) {
-			/* Got the mark, write it to the mbuf and return */
-			mbuf->hash.fdir.hi = mark_id;
-			mbuf->udata64 = (cfa_code & 0xffffffffull) << 32;
-			mbuf->hash.fdir.id = rxcmp1->cfa_code;
-			mbuf->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
-			return;
-		}
+	rc = ulp_mark_db_mark_get(bp->ulp_ctx, gfid,
+				  cfa_code, &mark_id);
+	if (!rc) {
+		/* Got the mark, write it to the mbuf and return */
+		mbuf->hash.fdir.hi = mark_id;
+		mbuf->udata64 = (cfa_code & 0xffffffffull) << 32;
+		mbuf->hash.fdir.id = rxcmp1->cfa_code;
+		mbuf->ol_flags |= PKT_RX_FDIR | PKT_RX_FDIR_ID;
+		return;
 	}
 
 skip_mark: