| Message ID | 5ce9986a-4c5c-9ffd-e83d-e6782ff370ba@solarflare.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dc8d2512e697f1f4d07b4722a5ca3b1bc84759e2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: further EF100 encap TSO features | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 56 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Nov 12, 2020 at 7:23 AM Edward Cree <ecree@solarflare.com> wrote: > > Our TSO descriptors got even more fussy. > > Signed-off-by: Edward Cree <ecree@solarflare.com> > --- > drivers/net/ethernet/sfc/bitfield.h | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/bitfield.h b/drivers/net/ethernet/sfc/bitfield.h > index 64731eb5dd56..1f981dfe4bdc 100644 > --- a/drivers/net/ethernet/sfc/bitfield.h > +++ b/drivers/net/ethernet/sfc/bitfield.h > @@ -289,7 +289,9 @@ typedef union efx_oword { > field14, value14, \ > field15, value15, \ > field16, value16, \ > - field17, value17) \ > + field17, value17, \ > + field18, value18, \ > + field19, value19) \ > (EFX_INSERT_FIELD_NATIVE((min), (max), field1, (value1)) | \ > EFX_INSERT_FIELD_NATIVE((min), (max), field2, (value2)) | \ > EFX_INSERT_FIELD_NATIVE((min), (max), field3, (value3)) | \ > @@ -306,7 +308,9 @@ typedef union efx_oword { > EFX_INSERT_FIELD_NATIVE((min), (max), field14, (value14)) | \ > EFX_INSERT_FIELD_NATIVE((min), (max), field15, (value15)) | \ > EFX_INSERT_FIELD_NATIVE((min), (max), field16, (value16)) | \ > - EFX_INSERT_FIELD_NATIVE((min), (max), field17, (value17))) > + EFX_INSERT_FIELD_NATIVE((min), (max), field17, (value17)) | \ > + EFX_INSERT_FIELD_NATIVE((min), (max), field18, (value18)) | \ > + EFX_INSERT_FIELD_NATIVE((min), (max), field19, (value19))) > > #define EFX_INSERT_FIELDS64(...) \ > cpu_to_le64(EFX_INSERT_FIELDS_NATIVE(__VA_ARGS__)) > @@ -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, ...) \ > @@ -391,7 +399,11 @@ typedef union efx_oword { > EFX_DWORD_3, 0xffffffff) > > /* Populate a quadword field with various numbers of arguments */ > -#define EFX_POPULATE_QWORD_17 EFX_POPULATE_QWORD > +#define EFX_POPULATE_QWORD_19 EFX_POPULATE_QWORD > +#define EFX_POPULATE_QWORD_18(qword, ...) \ > + EFX_POPULATE_QWORD_19(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) > +#define EFX_POPULATE_QWORD_17(qword, ...) \ > + EFX_POPULATE_QWORD_18(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) > #define EFX_POPULATE_QWORD_16(qword, ...) \ > EFX_POPULATE_QWORD_17(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) > #define EFX_POPULATE_QWORD_15(qword, ...) \ > @@ -432,7 +444,11 @@ typedef union efx_oword { > EFX_DWORD_1, 0xffffffff) > > /* Populate a dword field with various numbers of arguments */ > -#define EFX_POPULATE_DWORD_17 EFX_POPULATE_DWORD > +#define EFX_POPULATE_DWORD_19 EFX_POPULATE_DWORD > +#define EFX_POPULATE_DWORD_18(dword, ...) \ > + EFX_POPULATE_DWORD_19(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) > +#define EFX_POPULATE_DWORD_17(dword, ...) \ > + EFX_POPULATE_DWORD_18(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) > #define EFX_POPULATE_DWORD_16(dword, ...) \ > EFX_POPULATE_DWORD_17(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) > #define EFX_POPULATE_DWORD_15(dword, ...) \ > 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.
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. 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. -ed
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.
diff --git a/drivers/net/ethernet/sfc/bitfield.h b/drivers/net/ethernet/sfc/bitfield.h index 64731eb5dd56..1f981dfe4bdc 100644 --- a/drivers/net/ethernet/sfc/bitfield.h +++ b/drivers/net/ethernet/sfc/bitfield.h @@ -289,7 +289,9 @@ typedef union efx_oword { field14, value14, \ field15, value15, \ field16, value16, \ - field17, value17) \ + field17, value17, \ + field18, value18, \ + field19, value19) \ (EFX_INSERT_FIELD_NATIVE((min), (max), field1, (value1)) | \ EFX_INSERT_FIELD_NATIVE((min), (max), field2, (value2)) | \ EFX_INSERT_FIELD_NATIVE((min), (max), field3, (value3)) | \ @@ -306,7 +308,9 @@ typedef union efx_oword { EFX_INSERT_FIELD_NATIVE((min), (max), field14, (value14)) | \ EFX_INSERT_FIELD_NATIVE((min), (max), field15, (value15)) | \ EFX_INSERT_FIELD_NATIVE((min), (max), field16, (value16)) | \ - EFX_INSERT_FIELD_NATIVE((min), (max), field17, (value17))) + EFX_INSERT_FIELD_NATIVE((min), (max), field17, (value17)) | \ + EFX_INSERT_FIELD_NATIVE((min), (max), field18, (value18)) | \ + EFX_INSERT_FIELD_NATIVE((min), (max), field19, (value19))) #define EFX_INSERT_FIELDS64(...) \ cpu_to_le64(EFX_INSERT_FIELDS_NATIVE(__VA_ARGS__)) @@ -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, ...) \ @@ -391,7 +399,11 @@ typedef union efx_oword { EFX_DWORD_3, 0xffffffff) /* Populate a quadword field with various numbers of arguments */ -#define EFX_POPULATE_QWORD_17 EFX_POPULATE_QWORD +#define EFX_POPULATE_QWORD_19 EFX_POPULATE_QWORD +#define EFX_POPULATE_QWORD_18(qword, ...) \ + EFX_POPULATE_QWORD_19(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) +#define EFX_POPULATE_QWORD_17(qword, ...) \ + EFX_POPULATE_QWORD_18(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) #define EFX_POPULATE_QWORD_16(qword, ...) \ EFX_POPULATE_QWORD_17(qword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) #define EFX_POPULATE_QWORD_15(qword, ...) \ @@ -432,7 +444,11 @@ typedef union efx_oword { EFX_DWORD_1, 0xffffffff) /* Populate a dword field with various numbers of arguments */ -#define EFX_POPULATE_DWORD_17 EFX_POPULATE_DWORD +#define EFX_POPULATE_DWORD_19 EFX_POPULATE_DWORD +#define EFX_POPULATE_DWORD_18(dword, ...) \ + EFX_POPULATE_DWORD_19(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) +#define EFX_POPULATE_DWORD_17(dword, ...) \ + EFX_POPULATE_DWORD_18(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) #define EFX_POPULATE_DWORD_16(dword, ...) \ EFX_POPULATE_DWORD_17(dword, EFX_DUMMY_FIELD, 0, __VA_ARGS__) #define EFX_POPULATE_DWORD_15(dword, ...) \
Our TSO descriptors got even more fussy. Signed-off-by: Edward Cree <ecree@solarflare.com> --- drivers/net/ethernet/sfc/bitfield.h | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)