[dpdk-dev] [PATCH] net/bnxt: allow the mark to use a cfa code of zero

[dpdk-dev] [PATCH] net/bnxt: allow the mark to use a cfa code of zero

[Mike Baucom]

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(-)

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

Re: [dpdk-dev] [PATCH] net/bnxt: allow the mark to use a cfa code of zero

[Ajit Khaparde]

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

Re: [dpdk-dev] [PATCH] net/bnxt: allow the mark to use a cfa code of zero

[Ferruh Yigit]

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

Re: [dpdk-dev] [PATCH] net/bnxt: allow the mark to use a cfa code of zero

[Ajit Khaparde]

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