Re: [PATCH net-next 1/3] sfc: extend bitfield macros to 19 fields

[Alexander Duyck <alexander.duyck@gmail.com>]

On Mon, Nov 16, 2020 at 4:27 AM Edward Cree <ecree@solarflare.com> wrote:
>
> On 13/11/2020 19:06, Alexander Duyck wrote:
> > On Thu, Nov 12, 2020 at 7:23 AM Edward Cree <ecree@solarflare.com> wrote:
> >> @@ -348,7 +352,11 @@ typedef union efx_oword {
> >>  #endif
> >>
> >>  /* Populate an octword field with various numbers of arguments */
> >> -#define EFX_POPULATE_OWORD_17 EFX_POPULATE_OWORD
> >> +#define EFX_POPULATE_OWORD_19 EFX_POPULATE_OWORD
> >> +#define EFX_POPULATE_OWORD_18(oword, ...) \
> >> +       EFX_POPULATE_OWORD_19(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
> >> +#define EFX_POPULATE_OWORD_17(oword, ...) \
> >> +       EFX_POPULATE_OWORD_18(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
> >>  #define EFX_POPULATE_OWORD_16(oword, ...) \
> >>         EFX_POPULATE_OWORD_17(oword, EFX_DUMMY_FIELD, 0, __VA_ARGS__)
> >>  #define EFX_POPULATE_OWORD_15(oword, ...) \
> > Are all these macros really needed? It seems like this is adding a
> > bunch of noise in order to add support for a few additional fields.
> > Wouldn't it be possible to just define the ones that are actually
> > needed and add multiple dummy values to fill in the gaps instead of
> > defining every macro between zero and 19? For example this patch set
> > adds an option for setting 18 fields, but from what I can tell it is
> > never used.
> I guess the reasoningoriginally was that it's easier to read and
>  v-lint if it's just n repetitions of the same pattern.  Whereas if
>  there were jumps, it'd be more likely for a typo to slip through
>  unnoticed and subtly corrupt all the values.

I'm not sure the typo argument holds much water. The fact is it is
pretty easy to just count the variables doing something like the
following:
#define EFX_POPULATE_OWORD_10(oword, ...) \
        EFX_POPULATE_OWORD_19(oword, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              EFX_DUMMY_FIELD, 0, \
                              __VA_ARGS__)

Any change is basically update the 19 to whatever and add/subtract
lines using a simple copy/paste.

> But tbh I don't know, it's been like that since the driver was added
>  twelve years ago (8ceee660aacb) when it had all from 0 to 10.  All
>  we've done since then is extend that pattern.

The reason I bring it up is that it seems like it is dragging  a bunch
of macros that will likely never need 19 variables forward along with
it. For example the EFX_POPULATE_[DQ]WORD_<n> seems to only go as high
as 7. I'm not sure it makes much sense to keep defining new versions
of the macro when you could just be adding the needed lines to the 7
variable version of the macro and and come up with something that
looks like the definition of EFX_SET_OWORD where you could just add an
EFX_DUMMY_FIELD, 0, for each new variable added. The
EFX_POPULATE_OWORD_<X> goes all the way to 17 currently, however if we
exclude that the real distribution seems to be 1 - 10, with just the
one lone call to the 17 case which is becoming 19 with your patch.

The one issue I can think of is the fact that you will need the 17
variable version until you change it to the 19, but even then dropping
the 17 afterwards and adding the 2 additional sets of dummy variables
to the 10 should be straight forward and still pretty easy to review.