[ovs-dev] [PATCH v3 0/2] conntrack : Add support for rx chceksum offload.
Chandran, Sugesh
sugesh.chandran at intel.com
Tue Jul 11 12:29:32 UTC 2017
Hi Darrell,
Please see my comments below,
Regards
_Sugesh
> -----Original Message-----
> From: Darrell Ball [mailto:dball at vmware.com]
> Sent: Tuesday, July 11, 2017 1:19 AM
> To: Chandran, Sugesh <sugesh.chandran at intel.com>; Ilya Maximets
> <i.maximets at samsung.com>; ovs-dev at openvswitch.org; Ben Pfaff
> <blp at ovn.org>
> Subject: Re: [ovs-dev] [PATCH v3 0/2] conntrack : Add support for rx
> chceksum offload.
>
> Hi Sugesh
>
> For future reference, the naming guidelines are actually pretty flexible in this
> regard.
>
> Note that the cover letter will not get merged anyways, so it is moot.
[Sugesh] Sure, Thanks for that!
>
> Can you
>
> 1) Split out all the changes to the dp-packet module as Patch 1.
> You can say the new ‘*_bad’ APIs will get their first user in Patch 2
[Sugesh] there were comments in some of earlier patch series
to keep the caller and calle in the same commit for easy bisect and understand
the changes from the commit easily. I put the _bad* into patch-2 due to this reason.
In this patch series, do you think , its fine to introduce a unused function in patch-1(May get warning
for unused function) and use it later in the patch-2?.
Will send out the v5 accordingly.
> 2) Add me as co-author as the additions b/w V2 and V4 were significant.
[Sugesh] I wanted to add your name in co-auther at v3 itself. Didn’t do that as you were reviewing the changes :)
Will add your name in the next version.
>
> Thanks Darrell
>
>
>
>
>
> On 7/10/17, 4:51 PM, "Chandran, Sugesh" <sugesh.chandran at intel.com>
> wrote:
>
> Hi Darell, Ilya
>
> Regards
> _Sugesh
>
>
> > -----Original Message-----
> > From: Darrell Ball [mailto:dball at vmware.com]
> > Sent: Monday, July 10, 2017 5:53 PM
> > To: Ilya Maximets <i.maximets at samsung.com>; ovs-
> dev at openvswitch.org;
> > Ben Pfaff <blp at ovn.org>
> > Cc: Chandran, Sugesh <sugesh.chandran at intel.com>; Traynor, Kevin
> > <kevin.traynor at intel.com>
> > Subject: Re: [ovs-dev] [PATCH v3 0/2] conntrack : Add support for rx
> > chceksum offload.
> >
> >
> >
> > On 7/10/17, 1:37 AM, "Ilya Maximets" <i.maximets at samsung.com>
> wrote:
> >
> > > ‘chceksum’ is misspelled
> > >
> > > Since these patches really only affect ‘dpdk’, the module name
> ‘dpdk’
> > may more accurately
> > > reflect the real effect of these patches.
> >
> > Please, don't do that. Only patches that changes lib/dpdk.{c,h} should
> > have 'dpdk' prefix in subject line. All other patches should have proper
> > module name according to code they're changing.
> >
> > I wanted to rise this issue many times ago. So, maybe it's time.
> > There are many places where changes made to improve the DPDK-
> enabled
> > datapath, but the most of changes are generic and doesn't have many
> > DPDK-related code. Such patches doesn't need to have 'dpdk' as a
> prefix.
> > This only makes a mess from the git history and you can never say for
> > sure what module was changed in a particular patch by looking only on
> its
> > subject.
> >
> > These changes affect only the dpdk datapath.
> > I gave a full response to Sugesh.
> >
> >
> > IMHO, patches should have prefixes according to modules they're
> changing
> > like it is described in contribution guide. Generic changes should be
> > reviewed by not only people interested in DPDK. Addition of such
> > misleading prefixes forces them to miss maybe important generic
> changes.
> >
> > In this case, the module name is misleading since the changes affect
> much
> > more than just conntrack; that is the point.
> > The changes affect generic checksum offloading by virtue of changes to
> dp-
> > packet.
> > These changes are in fact specific to dpdk.
> >
> >
> > From the other side, many people adds 'dpdk' prefix to patches
> targeted
> > to 'netdev-dpdk' which is not right too.
> > All patches should have the right prefix according to the module they
> are
> > trying to change. That is my point of view.
> >
> >
> > In this particular case patches actually adds generic functionality
> > which can be used even without DPDK. For example, if we'll implement
> > checksum offloading for netdev-linux (not so hard). DPDK already
> > mentioned
> > in commit message as the target and there is no need for misleading
> > prefixes.
> >
> > That is not correct.
> > Here in the present, these changes are specific to dpdk. We work with
> the
> > present not one possible hypotectical future.
> > ‘IF’, in future, other code is changed that allows sharing of some code
> > changes beyond dpdk, then discussion of netdev-linux becomes
> relevant. It
> > is not relevant now.
> >
> >
> [Sugesh]I sent out the v4 patch already with same conntrack subject line
> considering that’s the guideline.
> I don’t mind changing that to dpdk, if we are following the practice to keep
> subject line based on the patch specific to what.
>
> > Best regards, Ilya Maximets.
> >
>
>
More information about the dev
mailing list