[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