From: Mahesh Bandewar <maheshb@google.com>
To: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: linux-netdev <netdev@vger.kernel.org>,
Ben Hutchings <bhutchings@solarflare.com>,
David Miller <davem@davemloft.net>
Subject: Re: extending feature word.
Date: Mon, 11 Apr 2011 11:45:05 -0700 [thread overview]
Message-ID: <BANLkTimFzTGKmhfRxzEUt_LwrnhAaetU5Q@mail.gmail.com> (raw)
In-Reply-To: <20110410101954.GA23753@rere.qmqm.pl>
On Sun, Apr 10, 2011 at 3:19 AM, Michał Mirosław
<mirq-linux@rere.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@rere.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..
next prev parent reply other threads:[~2011-04-11 18:45 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-02 2:07 extending feature word Mahesh Bandewar
2011-04-02 12:42 ` Michał Mirosław
2011-04-03 3:09 ` Mahesh Bandewar
2011-04-05 11:30 ` Michał Mirosław
2011-04-05 12:07 ` Ben Hutchings
2011-04-05 22:15 ` Mahesh Bandewar
2011-04-08 10:05 ` Michał Mirosław
2011-04-08 18:17 ` Mahesh Bandewar
2011-04-10 10:19 ` Michał Mirosław
2011-04-11 18:45 ` Mahesh Bandewar [this message]
2011-04-11 18:54 ` Stephen Hemminger
2011-04-11 19:16 ` Mahesh Bandewar
2011-04-11 19:19 ` Michał Mirosław
2011-04-11 19:49 ` Stephen Hemminger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BANLkTimFzTGKmhfRxzEUt_LwrnhAaetU5Q@mail.gmail.com \
--to=maheshb@google.com \
--cc=bhutchings@solarflare.com \
--cc=davem@davemloft.net \
--cc=mirq-linux@rere.qmqm.pl \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).