Re: [PATCHv3] net: Define enum for the bits used in features.

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jun 06, 2011 at 11:35:15PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 06, 2011 at 12:20:59PM -0700, David Miller wrote:
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > Date: Mon, 6 Jun 2011 18:32:53 +0300
> > 
> > > On Sun, Jun 05, 2011 at 10:15:37PM -0700, David Miller wrote:
> > >> Since the GSO accessors deal with mutliple bits, you can create
> > >> special GSO specific interfaces to manipulate them.
> > > 
> > > Yes but it's not just GSO.
> > > It's anything that includes more than 1 feature.
> > > Examples:
> > > NETIF_F_ALL_CSUM
> > > NETIF_F_ALL_TX_OFFLOADS
> > > NETIF_F_V6_CSUM
> > > NETIF_F_SOFT_FEATURES
> > > 
> > > etc
> > > 
> > > Creating many accessors for each will need a lot
> > > of code duplication ...
> > 
> > Yet this is something you must resolve in order to change the feature
> > bit implementation.
> > 
> > Whether this issue is difficult or not to address, it has to be done
> > either way.
> 
> I think I found a truly elegant solution to this
> problem which this margin is too narrow to contain ...

OK, it looks like using variadic macros from C99 makes this
possible, even though use of ungarded comma in macros
below makes me cringe:

/* Set all bits in the first 64 arguments, ignore the rest */
#define NETIF_F_OR_64( \
	_000, _001 , _002 , _003 , _004 , _005 , _006 , _007, \
	_010, _011 , _012 , _013 , _014 , _015 , _016 , _017, \
	_020, _021 , _022 , _023 , _024 , _025 , _026 , _027, \
	_030, _031 , _032 , _033 , _034 , _035 , _036 , _037, \
	_040, _041 , _042 , _043 , _044 , _045 , _046 , _047, \
	_050, _051 , _052 , _053 , _054 , _055 , _056 , _057, \
	_060, _061 , _062 , _063 , _064 , _065 , _066 , _067, \
	_070, _071 , _072 , _073 , _074 , _075 , _076 , _077, \
	... ) \
	((_000) | (_001)  | (_002)  | (_003)  | (_004)  | (_005)  | (_006)  | (_007) | \
	 (_010) | (_011)  | (_012)  | (_013)  | (_014)  | (_015)  | (_016)  | (_017) | \
	 (_020) | (_021)  | (_022)  | (_023)  | (_024)  | (_025)  | (_026)  | (_027) | \
	 (_030) | (_031)  | (_032)  | (_033)  | (_034)  | (_035)  | (_036)  | (_037) | \
	 (_040) | (_041)  | (_042)  | (_043)  | (_044)  | (_045)  | (_046)  | (_047) | \
	 (_050) | (_051)  | (_052)  | (_053)  | (_054)  | (_055)  | (_056)  | (_057) | \
	 (_060) | (_061)  | (_062)  | (_063)  | (_064)  | (_065)  | (_066)  | (_067) | \
	 (_070) | (_071)  | (_072)  | (_073)  | (_074)  | (_075)  | (_076)  | (_077) )

/* Verify that argument #65 is zero */
#define NETIF_F_BUG_ON_64( \
	_000, _001 , _002 , _003 , _004 , _005 , _006 , _007, \
	_010, _011 , _012 , _013 , _014 , _015 , _016 , _017, \
	_020, _021 , _022 , _023 , _024 , _025 , _026 , _027, \
	_030, _031 , _032 , _033 , _034 , _035 , _036 , _037, \
	_040, _041 , _042 , _043 , _044 , _045 , _046 , _047, \
	_050, _051 , _052 , _053 , _054 , _055 , _056 , _057, \
	_060, _061 , _062 , _063 , _064 , _065 , _066 , _067, \
	_070, _071 , _072 , _073 , _074 , _075 , _076 , _077, \
	_100, ... ) \
	BUG_ON((_100))

/* Set multiple bits in f. At most 64 bits can be
 * set in this way.
 * Nested calls are padded with 0 arguments
 * to ensure there are at least 64 of them */
#define NETIF_F_INIT(f, ...) do { \
	f |= NETIF_F_OR_64(__VA_ARGS__, \
			0, 0, 0, 0, 0, 0, 0, 0,\
			0, 0, 0, 0, 0, 0, 0, 0,\
			0, 0, 0, 0, 0, 0, 0, 0,\
			0, 0, 0, 0, 0, 0, 0, 0,\
			0, 0, 0, 0, 0, 0, 0, 0,\
			0, 0, 0, 0, 0, 0, 0, 0,\
			0, 0, 0, 0, 0, 0, 0, 0,\
			0, 0, 0, 0, 0, 0, 0, 0 \
		       ); \
	NETIF_F_BUG_ON_64(__VA_ARGS__, \
		      0, 0, 0, 0, 0, 0, 0, 0,\
		      0, 0, 0, 0, 0, 0, 0, 0,\
		      0, 0, 0, 0, 0, 0, 0, 0,\
		      0, 0, 0, 0, 0, 0, 0, 0,\
		      0, 0, 0, 0, 0, 0, 0, 0,\
		      0, 0, 0, 0, 0, 0, 0, 0,\
		      0, 0, 0, 0, 0, 0, 0, 0,\
		      0, 0, 0, 0, 0, 0, 0, 0 \
		     ); \
} while (0)

And now:
#define NETIF_F_GSO_SOFTWARE	 NETIF_F_TSO , NETIF_F_TSO_ECN , \
				 NETIF_F_TSO6 , NETIF_F_UFO


which makes

	NETIF_F_INIT(z, NETIF_F_GSO_SOFTWARE);

work as expected, and set all necessary bits,
so all we need to do is replace
	z = NETIF_F_GSO_SOFTWARE;
with call to macro above.

At most 64 different bits can be passed in this way
but NETIF_F_BUG_ON_64 above checks that.
If we want more than 64 bits, we just update
these macro definitions.

It seems that behaviour above is guaranteed by the language spec,
specifically the argument prescan rule.
Any C99 experts want to comment on this?

I have my doubts about whether the above is way too clever
even if it works. What do others think?


> -- 
> MST