netdev - Re: extending feature word.

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <BANLkTimFzTGKmhfRxzEUt_LwrnhAaetU5Q@mail.gmail.com>
Date:	Mon, 11 Apr 2011 11:45:05 -0700
From:	Mahesh Bandewar <maheshb@...gle.com>
To:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:	linux-netdev <netdev@...r.kernel.org>,
	Ben Hutchings <bhutchings@...arflare.com>,
	David Miller <davem@...emloft.net>
Subject: Re: extending feature word.

On Sun, Apr 10, 2011 at 3:19 AM, Michał Mirosław
<mirq-linux@...e.qmqm.pl> wrote:
> On Fri, Apr 08, 2011 at 11:17:41AM -0700, Mahesh Bandewar wrote:
>> On Fri, Apr 8, 2011 at 3:05 AM, Michał Mirosław <mirq-linux@...e.qmqm.pl> wrote:
>> > On Fri, Apr 01, 2011 at 07:07:05PM -0700, Mahesh Bandewar wrote:
>> >> Thanks for your comments on my loop-back patch. I was looking at the
>> >> code today from the perspective of extending various "features" for
>> >> word to an array of words and as Michael has pointed out, it's a huge
>> >> change. So I'm thinking on the following lines
>> >> (include/linux/netdevice.h)
>> >>
>> >> +#define DEV_FEATURE_WORDS      2
>> >> +#define LEGACY_FEATURE_WORD    0
>> >>        /* currently active device features */
>> >> -       u32                     features;
>> >> +       u32                     features[DEV_FEATURE_WORDS];
>> >>        /* user-changeable features */
>> >> -       u32                     hw_features;
>> >> +       u32                     hw_features[DEV_FEATURE_WORDS];
>> >>        /* user-requested features */
>> >> -       u32                     wanted_features;
>> >> +       u32                     wanted_features[DEV_FEATURE_WORDS];
>> >>        /* VLAN feature mask */
>> >> -       u32                     vlan_features;
>> >> +       u32                     vlan_features[DEV_FEATURE_WORDS];
>> >
>> > Hmm. There might be no point in making features field an array.
>> > This gives us nothing really. Maybe just add features_2 or similar?
>> > If we ever get to the point there need to be more than two words for
>> > features we can think of some abstraction layer then.
>> >
>> That is right! making it an array doesn't really buy us anything
>> unless there is a uniform way of managing all the bits spread across
>> multiple words inside that array. This was the reason why I have
>> changed that array into a bitmap in the patch that I have posted
>> earlier. This way the upper limit (currently only 32 bits) will be
>> removed and we'll have a long term solution. There will be little bit
>> of work involved but 'doing-things-right' has cost associated.
>
> I really don't like the bitmap idea. It multiplies the amount of code
> needed to manipulate multiple bits at once --- and that's a common
> thing for drivers to do. Almost every driver that needs ndo_fix_features
> will clear sets --- checkumming set, TSO set, all TX offloads set, ...
>
Should the added code be of any concern? If that is happening in the
control-path and does not affect the data-path as such; those added
instructions is a cost of added flexibility to we got through bitmap.
If performance is not at risk then that shouldn't be a problem.

> As a first step just add another set of words:
>
> union {
>        struct {
>                u32 features;
>                u32 features_2;
>        } /* anonymous struct */;
>        u32 features_array[2];
> } /* anonymous union */;
>
> This allows to change drivers after core supporting code gets implemented.
>
I agree that this will be an easier path as far as the extension of
features is concerned. Though this still does not simplify the
management of bits that are spanning across multiple words? Also
atomicity of those operations?

Thanks,
--mahesh..
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html