[ovs-dev] [PATCH v3 0/2] conntrack : Add support for rx chceksum offload.

[Darrell Ball]

On 7/10/17, 3:28 AM, "Chandran, Sugesh" <sugesh.chandran at intel.com> wrote:

    
    
    Regards
    _Sugesh
    
    
    > -----Original Message-----
    > From: Ilya Maximets [mailto:i.maximets at samsung.com]
    > Sent: Monday, July 10, 2017 9:37 AM
    > To: ovs-dev at openvswitch.org; Darrell Ball <dball at vmware.com>; 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.
    > 
    > > ‘chceksum’ is misspelled
    > >
    [Sugesh] Will correct in next version.
    > > 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.
    > 
    > 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.
    > 
    > 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.
    > 
    > Best regards, Ilya Maximets.
    [Sugesh] I put the subject line as conntrack with the same view as Ilya.
    I assume it is guideline for the subject line. Darrel , Do you think otherwise?

This series affects code outside of the conntrack.c file because it changes generic DPDK
checksum offloading by virtue of changes to the dp-packet module; these changes
are specific to DPDK. A quick look at the code makes that obvious.

Additionally, the code changes in conntack.c affect ‘only DPDK’, here in the present,
not some possible hypotectical future.

The common theme here is dpdk across both the dp-packet module and conntrack.c.

If the changes affected only conntrack.c, then it might be ok to use conntrack
as the module name.

The module name conntrack for this series is misleading as it misses the big picture.

I don’t want to hold this series for something so trivial, since all the developers I
know usually scan a list of full path file names of patches, not the ‘module’ name.
Let me check with a few folks.