netdev - Re: [net-next 08/15] i40e: Allow user to change input set mask for flow director

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 5 May 2016 08:00:21 -0700
From:	"Patil, Kiran" <kiran.patil@...el.com>
To:	John Fastabend <john.fastabend@...il.com>,
	David Miller <davem@...emloft.net>, jeffrey.t.kirsher@...el.com
Cc:	netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
	jogreene@...hat.com
Subject: Re: [net-next 08/15] i40e: Allow user to change input set mask for
 flow director

On 5/4/2016 8:22 PM, John Fastabend wrote:
> On 16-05-04 07:47 PM, Patil, Kiran wrote:
>> On 4/26/2016 8:48 PM, David Miller wrote:
>>> From: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
>>> Date: Tue, 26 Apr 2016 13:55:41 -0700
>>>
>>>> From: Kiran Patil <kiran.patil@...el.com>
>>>>
>>>> This patch implements feature, which allows user to change
>>>> input set mask for flow director using side-band channel.
>>>> This patch adds definition of FLOW_TYPE_MASK into the header file.
>>>> With this patch, user can now specify less than 4 tuple(src ip, dsp ip,
>>>> src port, dst port) for flow type TCP4/UDP4.
>>>>
>>>> Change-Id: I90052508f8c172c0da5a4b78b949704b4a59ea78
>>>> Signed-off-by: Kiran Patil <kiran.patil@...el.com>
>>>> Tested-by: Andrew Bowers <andrewx.bowers@...el.com>
>>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
>>>
>>> If you want to do this, you have to define the semantics generically
>>> in include/uapi/linux/ethtool.h so that other drivers can implement
>>> this too.
>>>
>>> Please don't ever implement private, driver specific, interpretations
>>> of the generic ethtool facilitites.
>>>
>>> The semantics and interpretations of the values must absolutely be
>>> consistent across all drivers in the tree.
>>>
>>> Otherwise the user experience is terrible.
>>>
>>> Thanks.
>>>
>> This is not new feature implemented in i40e driver. This is the original
>> feature of ethtool which allows user to specify subset of tuple for
>> setting up flow director.
>>
>> i40e driver using it same way as ixgbe.
>>
>> Please let us know if I misinterpreted your response.
>>
>> Would you recommend that we re-submit patch with better patch
>> description (indicating that it is not new feature but just enabling
>> feature).
>>
>> Thanks,
>> -- Kiran P.
>
> At least define FLOW_TYPE_MASK in ethtool.h then its not
> sort of hobbled together in the driver and others can use it
> instead of the normal ~FLOW_EXT which I see other drivers used.
> Another benefit if its near the definition of the flow types
> you have a chance of someone seeing it if they add a flag
> past 0xff.
>
> And maybe do it as a separate patch. So you aren't adding normal
> driver ethtool implementation and a new #define for all drivers
> in the same patch.
>
> .John
>
Now I see what the problem is (FLOW_TYPE_MASK). Unfortunately it was 
never meant to be part of this patch since it was introduced by other 
patch (Cloud filter support) and was suggested to move into this header 
file. As suggested, we can create another patch just for this 
FLOW_TYPE_MASK in ethtool.h and re-work original patch to not have 
define for FLOW_TYPE_MASK.

May be we can go back and think, it this define absolutely needed or not.

Thanks,
-- Kiran P.