extending feature word.

extending feature word.

[Mahesh Bandewar]

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];
+#define legacy_features                features[LEGACY_FEATURE_WORD]
+#define legacy_hw_features     hw_features[LEGACY_FEATURE_WORD]
+#define legacy_wanted_features wanted_features[LEGACY_FEATURE_WORD]
+#define legacy_vlan_features    vlan_features[LEGACY_FEATURE_WORD]


So that it will be a matter of -

s/features/legacy_features/
s/hw_features/legacy_hw_features/
s/wanted_features/legacy_wanted_features/
s/vlan_features/legacy_vlan_features/

to start with as a first step. Once this is done, legacy_* can be
changed with respective arrays and modified as per the need. Comments?

--mahesh..

Re: extending feature word.

[Michał Mirosław]

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];
> +#define legacy_features                features[LEGACY_FEATURE_WORD]
> +#define legacy_hw_features     hw_features[LEGACY_FEATURE_WORD]
> +#define legacy_wanted_features wanted_features[LEGACY_FEATURE_WORD]
> +#define legacy_vlan_features    vlan_features[LEGACY_FEATURE_WORD]
> 
> 
> So that it will be a matter of -
> 
> s/features/legacy_features/
> s/hw_features/legacy_hw_features/
> s/wanted_features/legacy_wanted_features/
> s/vlan_features/legacy_vlan_features/
> 
> to start with as a first step. Once this is done, legacy_* can be
> changed with respective arrays and modified as per the need. Comments?

You could just s/features/features[0]/ as well. Why the extra hiding?

If you want to split the work, it would be clearer to first convert
hw_features and wanted_features (with all the core code touching it -
this is the easy part), then vlan_features (this includes drivers'
and VLAN code) and then features (it's all over).

Best Regards,
Michał Mirosław

Re: extending feature word.

[Mahesh Bandewar]

On Sat, Apr 2, 2011 at 5:42 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];
>> +#define legacy_features                features[LEGACY_FEATURE_WORD]
>> +#define legacy_hw_features     hw_features[LEGACY_FEATURE_WORD]
>> +#define legacy_wanted_features wanted_features[LEGACY_FEATURE_WORD]
>> +#define legacy_vlan_features    vlan_features[LEGACY_FEATURE_WORD]
>>
>>
>> So that it will be a matter of -
>>
>> s/features/legacy_features/
>> s/hw_features/legacy_hw_features/
>> s/wanted_features/legacy_wanted_features/
>> s/vlan_features/legacy_vlan_features/
>>
>> to start with as a first step. Once this is done, legacy_* can be
>> changed with respective arrays and modified as per the need. Comments?
>
> You could just s/features/features[0]/ as well. Why the extra hiding?
>
I was thinking that features[0] is really a short term solution sine
there are only couple of bits left in this word and any new will have
to be added into the next word. So eventually it should be managed
properly at all these places where we'll replace it with features[0].
So it's a nice indication that this was the legacy way of dealing and
need to be altered for additional bits (especially where we see
legacy_* stuff) where ever required.

On the other hand replacing with features[0] breaks the NETDEVICE_SHOW
macro in net/core/net-sysfs.c and need to be expanded manually.

> If you want to split the work, it would be clearer to first convert
> hw_features and wanted_features (with all the core code touching it -
> this is the easy part), then vlan_features (this includes drivers'
> and VLAN code) and then features (it's all over).
>
I like the idea of splitting but it will be only useful when all of it
is done and not partially, isn't it? Or am I missing something?

> Best Regards,
> Michał Mirosław
>

Re: extending feature word.

[Michał Mirosław]

On Sat, Apr 02, 2011 at 08:09:14PM -0700, Mahesh Bandewar wrote:
> On Sat, Apr 2, 2011 at 5:42 AM, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> > On Fri, Apr 01, 2011 at 07:07:05PM -0700, Mahesh Bandewar wrote:
> > If you want to split the work, it would be clearer to first convert
> > hw_features and wanted_features (with all the core code touching it -
> > this is the easy part), then vlan_features (this includes drivers'
> > and VLAN code) and then features (it's all over).
> I like the idea of splitting but it will be only useful when all of it
> is done and not partially, isn't it? Or am I missing something?

Since this is a big change, when split it might be easier to follow.
OTOH, with your idea of macro it might be easier to do incremental
changes (I think this will be a lot of work for no gain in this case).

Best Regards,
Michał Mirosław

Re: extending feature word.

[Ben Hutchings]

On Tue, 2011-04-05 at 13:30 +0200, Michał Mirosław wrote:
> On Sat, Apr 02, 2011 at 08:09:14PM -0700, Mahesh Bandewar wrote:
> > On Sat, Apr 2, 2011 at 5:42 AM, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> > > On Fri, Apr 01, 2011 at 07:07:05PM -0700, Mahesh Bandewar wrote:
> > > If you want to split the work, it would be clearer to first convert
> > > hw_features and wanted_features (with all the core code touching it -
> > > this is the easy part), then vlan_features (this includes drivers'
> > > and VLAN code) and then features (it's all over).
> > I like the idea of splitting but it will be only useful when all of it
> > is done and not partially, isn't it? Or am I missing something?
> 
> Since this is a big change, when split it might be easier to follow.
> OTOH, with your idea of macro it might be easier to do incremental
> changes (I think this will be a lot of work for no gain in this case).

I strongly disagree with using macros for this.  They are very likely to
conflict with other identifiers..

We might be able to get away with something like:

	union {
		u32 features;
		u32 feature[N];
	};
	union {
		u32 vlan_features;
		u32 vlan_feature[N];
	};
	union {
		u32 hw_features;
		u32 hw_feature[N];
	};

(assuming hw_features is new enough that there is no need for the
alias).

Anyway, if we're going to put all the feature words in net_device
there's no longer any reason for NETIF_F_LOOPBACK not to be in the first
word.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: extending feature word.

[Mahesh Bandewar]

On Tue, Apr 5, 2011 at 5:07 AM, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Tue, 2011-04-05 at 13:30 +0200, Michał Mirosław wrote:
>> On Sat, Apr 02, 2011 at 08:09:14PM -0700, Mahesh Bandewar wrote:
>> > On Sat, Apr 2, 2011 at 5:42 AM, Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
>> > > On Fri, Apr 01, 2011 at 07:07:05PM -0700, Mahesh Bandewar wrote:
>> > > If you want to split the work, it would be clearer to first convert
>> > > hw_features and wanted_features (with all the core code touching it -
>> > > this is the easy part), then vlan_features (this includes drivers'
>> > > and VLAN code) and then features (it's all over).
>> > I like the idea of splitting but it will be only useful when all of it
>> > is done and not partially, isn't it? Or am I missing something?
>>
>> Since this is a big change, when split it might be easier to follow.
>> OTOH, with your idea of macro it might be easier to do incremental
>> changes (I think this will be a lot of work for no gain in this case).
>
> I strongly disagree with using macros for this.  They are very likely to
> conflict with other identifiers..
>
> We might be able to get away with something like:
>
>        union {
>                u32 features;
>                u32 feature[N];
>        };
>        union {
>                u32 vlan_features;
>                u32 vlan_feature[N];
>        };
>        union {
>                u32 hw_features;
>                u32 hw_feature[N];
>        };
>
unfortunately the members have to be of the same size to be part of a
name-less union. Otherwise you have to name it and add a macro to
provide backward compatibility. I did not find other occurrence of
"legacy_(hw/wanted/vlan)_features" if that was your only concern.

Having a macro is not the best thing and ideally I would want this to
live as little as possible. Since the use is so wide spread, having
the ability to make incremental changes is required. Once we switch to
the new method, hopefully we can eliminate this macro.

> (assuming hw_features is new enough that there is no need for the
> alias).
>
> Anyway, if we're going to put all the feature words in net_device
> there's no longer any reason for NETIF_F_LOOPBACK not to be in the first
> word.
>
Too late Ben, I think Tom already used the last available bit :)

> Ben.
>
> --
> Ben Hutchings, Senior Software Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
>

Re: extending feature word.

[Michał Mirosław]

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.

Or we might add a new field and put there NETIF_F_LLTX, NETIF_F_HIGHDMA
and others that are not user changeable ever. Those don't need dynamic
propagation to slave devices (e.g. VLAN) and wanted/hw_features for them.

Best Regards,
Michał Mirosław

Re: extending feature word.

[Mahesh Bandewar]

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.

> Or we might add a new field and put there NETIF_F_LLTX, NETIF_F_HIGHDMA
> and others that are not user changeable ever. Those don't need dynamic
> propagation to slave devices (e.g. VLAN) and wanted/hw_features for them.
>
This will certainly buy us some time but will be a temporary fix until
we runout of bits again. Also adding a second word (separate from the
first word) will create fragmentation and different approaches to
manage these two words and (I think) wont be desirable.

There will be another approach where we change this to u64 and
postpone the problem little longer and probably wait for u128 to make
it even longer. This is again a mid-term fix and not really a
solution.

In the patch that I have posted, I have changed these fiels to bitmaps
and a plan to take it there. This will _solve_ the problem once and
for all.


Thanks,
--mahesh..

> Best Regards,
> Michał Mirosław
>

Re: extending feature word.

[Michał Mirosław]

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, ...

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.

Best Regards,
Michał Mirosław

Re: extending feature word.

[Mahesh Bandewar]

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..

Re: extending feature word.

[Stephen Hemminger]

On Mon, 11 Apr 2011 11:45:05 -0700
Mahesh Bandewar <maheshb@google.com> wrote:

> >> 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.

Just to be dense... What is wrong with just using u64?

-- 

Re: extending feature word.

[Mahesh Bandewar]

On Mon, Apr 11, 2011 at 11:54 AM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> On Mon, 11 Apr 2011 11:45:05 -0700
> Mahesh Bandewar <maheshb@google.com> wrote:
>
>> >> 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.
>
> Just to be dense... What is wrong with just using u64?
>
I have already suggested that in this thread. With this theoretically
you removing one limit and imposing another and that's why I said it
would be a mid-term solution. But again by the time all 64 bits are
gone (got used), we may have u128 available.

> --
>

Re: extending feature word.

[Michał Mirosław]

On Mon, Apr 11, 2011 at 11:54:59AM -0700, Stephen Hemminger wrote:
> On Mon, 11 Apr 2011 11:45:05 -0700
> Mahesh Bandewar <maheshb@google.com> wrote:
> > >> 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.
> Just to be dense... What is wrong with just using u64?

Hmm. Looks like this is so simple that nobody thought of it seriously. ;)

This of course needs a bit of glue code in G/SFEATURES handling, but most
of the change would be s/u32/u64/ in apropriate places.

Best Regards,
Michał Mirosław

Re: extending feature word.

[Stephen Hemminger]

On Mon, 11 Apr 2011 21:19:47 +0200
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:

> On Mon, Apr 11, 2011 at 11:54:59AM -0700, Stephen Hemminger wrote:
> > On Mon, 11 Apr 2011 11:45:05 -0700
> > Mahesh Bandewar <maheshb@google.com> wrote:
> > > >> 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.
> > Just to be dense... What is wrong with just using u64?
> 
> Hmm. Looks like this is so simple that nobody thought of it seriously. ;)
> 
> This of course needs a bit of glue code in G/SFEATURES handling, but most
> of the change would be s/u32/u64/ in apropriate places.

I am a strong proponent of not building stuff until it is needed.
  http://www.extremeprogramming.org/rules/early.html
By the time 64 bits are exhausted the existing model of network device
may have changed significantly anyway.