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

[Somnath Kotur <somnath.kotur@broadcom.com>]

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