DPDK patches and discussions
 help / color / mirror / Atom feed
From: Somnath Kotur <somnath.kotur@broadcom.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] net/bnxt: Fix bug with duplicate filter pattern for flow director
Date: Fri, 23 Mar 2018 17:11:08 +0530	[thread overview]
Message-ID: <CAOBf=mumWvb-4-8Hb135LtPTt7uTskVwJy2-=s565qDd+3sKrw@mail.gmail.com> (raw)
In-Reply-To: <091eb993-5f3e-f56e-61f3-1b53ab409b5d@intel.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
>
>

      reply	other threads:[~2018-03-23 11:41 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-22  2:58 Somnath Kotur
2018-03-22 18:46 ` Ferruh Yigit
2018-03-23  3:34   ` Somnath Kotur
2018-03-23 10:41     ` Ferruh Yigit
2018-03-23 11:41       ` Somnath Kotur [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAOBf=mumWvb-4-8Hb135LtPTt7uTskVwJy2-=s565qDd+3sKrw@mail.gmail.com' \
    --to=somnath.kotur@broadcom.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).