| Message ID | 20171003200546.165731-1-mka@chromium.org |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | nfp: convert nfp_eth_set_bit_config() into a macro | expand |
On Tue, 3 Oct 2017 13:05:46 -0700, Matthias Kaehlcke wrote: > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to > identify the 'mask' parameter as known to be constant at compile time, > which is required to use the FIELD_GET() macro. > > The forced inlining does the trick for gcc, but for kernel builds with > clang it results in undefined symbols: > > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.o: In function > `__nfp_eth_set_aneg': > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x787): > undefined reference to `__compiletime_assert_492' > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x7b1): > undefined reference to `__compiletime_assert_496' > > These __compiletime_assert_xyx() calls would have been optimized away if > the compiler had seen 'mask' as a constant. > > Convert nfp_eth_set_bit_config() into a macro, which allows both gcc and > clang to identify 'mask' as a compile time constant. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> :( Is there no chance of fixing the constant propagation in the compiler?
El Tue, Oct 03, 2017 at 02:50:00PM -0700 Jakub Kicinski ha dit: > On Tue, 3 Oct 2017 13:05:46 -0700, Matthias Kaehlcke wrote: > > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to > > identify the 'mask' parameter as known to be constant at compile time, > > which is required to use the FIELD_GET() macro. > > > > The forced inlining does the trick for gcc, but for kernel builds with > > clang it results in undefined symbols: > > > > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.o: In function > > `__nfp_eth_set_aneg': > > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x787): > > undefined reference to `__compiletime_assert_492' > > drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x7b1): > > undefined reference to `__compiletime_assert_496' > > > > These __compiletime_assert_xyx() calls would have been optimized away if > > the compiler had seen 'mask' as a constant. > > > > Convert nfp_eth_set_bit_config() into a macro, which allows both gcc and > > clang to identify 'mask' as a compile time constant. > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > :( > > Is there no chance of fixing the constant propagation in the compiler? LLVM developers are reluctant and would like us kernel folks to evaluate possible alternatives for the affected code first: https://bugs.llvm.org/show_bug.cgi?id=4898 Given that this doesn't seem to be a widespread issue in the kernel personally I would consider the conversion to a macro in this case an acceptable solution, though it is definitely ugly. However I'm not the owner of the driver or the subsystem, so my opinion doesn't really carry much weight here ;-) Any ideas about other, less ugly alternatives? Matthias
From: Matthias Kaehlcke <mka@chromium.org> Date: Wed, 4 Oct 2017 10:42:15 -0700 > Given that this doesn't seem to be a widespread issue in the kernel > personally I would consider the conversion to a macro in this case an > acceptable solution, though it is definitely ugly. However I'm not the > owner of the driver or the subsystem, so my opinion doesn't really > carry much weight here ;-) Losing type checking is a serious regression as far as I am concerned.
On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote: > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to > identify the 'mask' parameter as known to be constant at compile time, > which is required to use the FIELD_GET() macro. > > The forced inlining does the trick for gcc, but for kernel builds with > clang it results in undefined symbols: Can't you use local different FIELD_PREP/FIELD_GET macros with a different name without the BUILD_BUG tests? i.e.: #define NFP_FIELD_PREP(_mask, _val) \ ({ \ ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ }) #define NFP_FIELD_GET(_mask, _reg) \ ({ \ (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \ }) Then the __always_inline can be removed from nfp_eth_set_bit_config too.
On Wed, 4 Oct 2017 10:42:42 -0700, Manoj Gupta wrote: > Hi Jakub, > > I had discussed about supporting this code with some clang developers. > However, the consensus was this code relies on a specific GCC optimizer > behavior and Clang does not share the same behavior by design. Hm. I find surprising that constant propagation to inlined functions is considered something GCC-specific rather than obvious/basic. > Note that even GCC can't compile this code at -O0. Yes, that makes me feel uncomfortable... Could you try this? -------8<------- diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c index f6f7c085f8e0..47251396fcae 100644 --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c @@ -469,10 +469,10 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed) return nfp_eth_config_commit_end(nsp); } -/* Force inline, FIELD_* macroes require masks to be compilation-time known */ -static __always_inline int +static int nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, - const u64 mask, unsigned int val, const u64 ctrl_bit) + const u64 mask, const unsigned int shift, + unsigned int val, const u64 ctrl_bit) { union eth_table_entry *entries = nfp_nsp_config_entries(nsp); unsigned int idx = nfp_nsp_config_idx(nsp); @@ -489,11 +489,11 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, /* Check if we are already in requested state */ reg = le64_to_cpu(entries[idx].raw[raw_idx]); - if (val == FIELD_GET(mask, reg)) + if (val == (reg & mask) >> shift) return 0; reg &= ~mask; - reg |= FIELD_PREP(mask, val); + reg |= (val << shift) & mask; entries[idx].raw[raw_idx] = cpu_to_le64(reg); entries[idx].control |= cpu_to_le64(ctrl_bit); @@ -503,6 +503,13 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, return 0; } +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit) \ + ({ \ + __BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \ + nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \ + val, ctrl_bit); \ + }) + /** * __nfp_eth_set_aneg() - set PHY autonegotiation control bit * @nsp: NFP NSP handle returned from nfp_eth_config_start() @@ -515,7 +522,7 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, */ int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode) { - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE, + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE, NSP_ETH_STATE_ANEG, mode, NSP_ETH_CTRL_SET_ANEG); } @@ -544,7 +551,7 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed) return -EINVAL; } - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE, + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE, NSP_ETH_STATE_RATE, rate, NSP_ETH_CTRL_SET_RATE); } @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed) */ int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes) { - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, lanes, NSP_ETH_CTRL_SET_LANES); }
El Wed, Oct 04, 2017 at 11:17:24AM -0700 Jakub Kicinski ha dit: > On Wed, 4 Oct 2017 10:42:42 -0700, Manoj Gupta wrote: > > Hi Jakub, > > > > I had discussed about supporting this code with some clang developers. > > However, the consensus was this code relies on a specific GCC optimizer > > behavior and Clang does not share the same behavior by design. > > Hm. I find surprising that constant propagation to inlined functions > is considered something GCC-specific rather than obvious/basic. > > > Note that even GCC can't compile this code at -O0. > > Yes, that makes me feel uncomfortable... Could you try this? The kernel as a whole does not build with -O0, so I couldn't try that. I know Manoj (who isn't a kernel dev) isolated the compiletime_assert macros and encountered the same 'undefined reference' errors with gcc -O0 that we are seeing with clang. This is due to: "if you use it in an inlined function and pass an argument of the function as the argument to the built-in, GCC never returns 1 when you call the inline function with a string constant or compound literal (see Compound Literals) and does not return 1 when you pass a constant numeric value to the inline function unless you specify the -O option." https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Other-Builtins.html > diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > index f6f7c085f8e0..47251396fcae 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > @@ -469,10 +469,10 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed) > return nfp_eth_config_commit_end(nsp); > } > > -/* Force inline, FIELD_* macroes require masks to be compilation-time known */ > -static __always_inline int > +static int > nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > - const u64 mask, unsigned int val, const u64 ctrl_bit) > + const u64 mask, const unsigned int shift, > + unsigned int val, const u64 ctrl_bit) > { > union eth_table_entry *entries = nfp_nsp_config_entries(nsp); > unsigned int idx = nfp_nsp_config_idx(nsp); > @@ -489,11 +489,11 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > > /* Check if we are already in requested state */ > reg = le64_to_cpu(entries[idx].raw[raw_idx]); > - if (val == FIELD_GET(mask, reg)) > + if (val == (reg & mask) >> shift) > return 0; > > reg &= ~mask; > - reg |= FIELD_PREP(mask, val); > + reg |= (val << shift) & mask; > entries[idx].raw[raw_idx] = cpu_to_le64(reg); > > entries[idx].control |= cpu_to_le64(ctrl_bit); > @@ -503,6 +503,13 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > return 0; > } > > +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit) \ > + ({ \ > + __BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \ > + nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \ > + val, ctrl_bit); \ > + }) > + > /** > * __nfp_eth_set_aneg() - set PHY autonegotiation control bit > * @nsp: NFP NSP handle returned from nfp_eth_config_start() > @@ -515,7 +522,7 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > */ > int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode) > { > - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE, > + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE, > NSP_ETH_STATE_ANEG, mode, > NSP_ETH_CTRL_SET_ANEG); > } > @@ -544,7 +551,7 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed) > return -EINVAL; > } > > - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE, > + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE, > NSP_ETH_STATE_RATE, rate, > NSP_ETH_CTRL_SET_RATE); > } > @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed) > */ > int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes) > { > - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, > + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, > lanes, NSP_ETH_CTRL_SET_LANES); > }
Hi Joe, El Wed, Oct 04, 2017 at 11:07:19AM -0700 Joe Perches ha dit: > On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote: > > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to > > identify the 'mask' parameter as known to be constant at compile time, > > which is required to use the FIELD_GET() macro. > > > > The forced inlining does the trick for gcc, but for kernel builds with > > clang it results in undefined symbols: > > Can't you use local different FIELD_PREP/FIELD_GET macros > with a different name without the BUILD_BUG tests? > > i.e.: > > #define NFP_FIELD_PREP(_mask, _val) \ > ({ \ > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > }) > > #define NFP_FIELD_GET(_mask, _reg) \ > ({ \ > (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \ > }) > > Then the __always_inline can be removed from > nfp_eth_set_bit_config too. Thanks for the suggestion. This seems a viable alternative if David and the NFP owners can live without the extra checking provided by __BF_FIELD_CHECK.
On Wed, 4 Oct 2017 11:49:57 -0700, Matthias Kaehlcke wrote: > Hi Joe, > > El Wed, Oct 04, 2017 at 11:07:19AM -0700 Joe Perches ha dit: > > > On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote: > > > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to > > > identify the 'mask' parameter as known to be constant at compile time, > > > which is required to use the FIELD_GET() macro. > > > > > > The forced inlining does the trick for gcc, but for kernel builds with > > > clang it results in undefined symbols: > > > > Can't you use local different FIELD_PREP/FIELD_GET macros > > with a different name without the BUILD_BUG tests? > > > > i.e.: > > > > #define NFP_FIELD_PREP(_mask, _val) \ > > ({ \ > > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > > }) > > > > #define NFP_FIELD_GET(_mask, _reg) \ > > ({ \ > > (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \ > > }) > > > > Then the __always_inline can be removed from > > nfp_eth_set_bit_config too. > > Thanks for the suggestion. This seems a viable alternative if David > and the NFP owners can live without the extra checking provided by > __BF_FIELD_CHECK. The reason the __BF_FIELD_CHECK refuses to compile non-constant masks is that it will require runtime ffs on the mask, which is potentially costly. I would also feel quite stupid adding those macros to the nfp driver, given that I specifically created the bitfield.h header to not have to reimplement these in every driver I write/maintain. Can you please test the patch I provided in the other reply?
Hi Jakub, El Wed, Oct 04, 2017 at 03:22:03PM -0700 Jakub Kicinski ha dit: > On Wed, 4 Oct 2017 11:49:57 -0700, Matthias Kaehlcke wrote: > > Hi Joe, > > > > El Wed, Oct 04, 2017 at 11:07:19AM -0700 Joe Perches ha dit: > > > > > On Tue, 2017-10-03 at 13:05 -0700, Matthias Kaehlcke wrote: > > > > nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to > > > > identify the 'mask' parameter as known to be constant at compile time, > > > > which is required to use the FIELD_GET() macro. > > > > > > > > The forced inlining does the trick for gcc, but for kernel builds with > > > > clang it results in undefined symbols: > > > > > > Can't you use local different FIELD_PREP/FIELD_GET macros > > > with a different name without the BUILD_BUG tests? > > > > > > i.e.: > > > > > > #define NFP_FIELD_PREP(_mask, _val) \ > > > ({ \ > > > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > > > }) > > > > > > #define NFP_FIELD_GET(_mask, _reg) \ > > > ({ \ > > > (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \ > > > }) > > > > > > Then the __always_inline can be removed from > > > nfp_eth_set_bit_config too. > > > > Thanks for the suggestion. This seems a viable alternative if David > > and the NFP owners can live without the extra checking provided by > > __BF_FIELD_CHECK. > > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks > is that it will require runtime ffs on the mask, which is potentially > costly. I would also feel quite stupid adding those macros to the nfp > driver, given that I specifically created the bitfield.h header to not > have to reimplement these in every driver I write/maintain. That make sense, thanks for providing more context. > Can you please test the patch I provided in the other reply? With this patch there are no errors when building the kernel with clang. Thanks! Matthias
On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote: > > > Thanks for the suggestion. This seems a viable alternative if David > > > and the NFP owners can live without the extra checking provided by > > > __BF_FIELD_CHECK. > > > > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks > > is that it will require runtime ffs on the mask, which is potentially > > costly. I would also feel quite stupid adding those macros to the nfp > > driver, given that I specifically created the bitfield.h header to not > > have to reimplement these in every driver I write/maintain. > > That make sense, thanks for providing more context. > > > Can you please test the patch I provided in the other reply? > > With this patch there are no errors when building the kernel with > clang. Cool, thanks for checking! I will run it through full tests and queue for upstreaming :)
Hi Jakub, On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote: >> > > Thanks for the suggestion. This seems a viable alternative if David >> > > and the NFP owners can live without the extra checking provided by >> > > __BF_FIELD_CHECK. >> > >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks >> > is that it will require runtime ffs on the mask, which is potentially >> > costly. I would also feel quite stupid adding those macros to the nfp >> > driver, given that I specifically created the bitfield.h header to not >> > have to reimplement these in every driver I write/maintain. >> >> That make sense, thanks for providing more context. >> >> > Can you please test the patch I provided in the other reply? >> >> With this patch there are no errors when building the kernel with >> clang. > > Cool, thanks for checking! I will run it through full tests and queue > for upstreaming :) Just to let you know, using __BF_FIELD_CHECK macro will not Link with -O0 (GCC or Clang) since references to __compiletime_assert_xxx will not be cleaned up. Thanks, Manoj
On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote: > On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote: > > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote: > >> > > Thanks for the suggestion. This seems a viable alternative if David > >> > > and the NFP owners can live without the extra checking provided by > >> > > __BF_FIELD_CHECK. > >> > > >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks > >> > is that it will require runtime ffs on the mask, which is potentially > >> > costly. I would also feel quite stupid adding those macros to the nfp > >> > driver, given that I specifically created the bitfield.h header to not > >> > have to reimplement these in every driver I write/maintain. > >> > >> That make sense, thanks for providing more context. > >> > >> > Can you please test the patch I provided in the other reply? > >> > >> With this patch there are no errors when building the kernel with > >> clang. > > > > Cool, thanks for checking! I will run it through full tests and queue > > for upstreaming :) > > Just to let you know, using __BF_FIELD_CHECK macro will not Link with > -O0 (GCC or Clang) since references to __compiletime_assert_xxx will > not be cleaned up. Do you mean the current nfp_eth_set_bit_config() will not work with -O0 on either complier, or any use of __BF_FIELD_CHECK() will not compile with -O0?
On Wed, Oct 4, 2017 at 5:56 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote: >> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote: >> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote: >> >> > > Thanks for the suggestion. This seems a viable alternative if David >> >> > > and the NFP owners can live without the extra checking provided by >> >> > > __BF_FIELD_CHECK. >> >> > >> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks >> >> > is that it will require runtime ffs on the mask, which is potentially >> >> > costly. I would also feel quite stupid adding those macros to the nfp >> >> > driver, given that I specifically created the bitfield.h header to not >> >> > have to reimplement these in every driver I write/maintain. >> >> >> >> That make sense, thanks for providing more context. >> >> >> >> > Can you please test the patch I provided in the other reply? >> >> >> >> With this patch there are no errors when building the kernel with >> >> clang. >> > >> > Cool, thanks for checking! I will run it through full tests and queue >> > for upstreaming :) >> >> Just to let you know, using __BF_FIELD_CHECK macro will not Link with >> -O0 (GCC or Clang) since references to __compiletime_assert_xxx will >> not be cleaned up. > > Do you mean the current nfp_eth_set_bit_config() will not work with -O0 > on either complier, or any use of __BF_FIELD_CHECK() will not compile > with -O0? Any use of __BF_FIELD_CHECK. The code will compile but not link since calls to ____compiletime_assert_xxx (added by compiletime_assert macro) will not be removed in -O0. Thanks, Manoj
On Wed, 4 Oct 2017 18:50:04 -0700, Manoj Gupta wrote: > On Wed, Oct 4, 2017 at 5:56 PM, Jakub Kicinski wrote: > > On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote: > >> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote: > >> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote: > >> >> > > Thanks for the suggestion. This seems a viable alternative if David > >> >> > > and the NFP owners can live without the extra checking provided by > >> >> > > __BF_FIELD_CHECK. > >> >> > > >> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks > >> >> > is that it will require runtime ffs on the mask, which is potentially > >> >> > costly. I would also feel quite stupid adding those macros to the nfp > >> >> > driver, given that I specifically created the bitfield.h header to not > >> >> > have to reimplement these in every driver I write/maintain. > >> >> > >> >> That make sense, thanks for providing more context. > >> >> > >> >> > Can you please test the patch I provided in the other reply? > >> >> > >> >> With this patch there are no errors when building the kernel with > >> >> clang. > >> > > >> > Cool, thanks for checking! I will run it through full tests and queue > >> > for upstreaming :) > >> > >> Just to let you know, using __BF_FIELD_CHECK macro will not Link with > >> -O0 (GCC or Clang) since references to __compiletime_assert_xxx will > >> not be cleaned up. > > > > Do you mean the current nfp_eth_set_bit_config() will not work with -O0 > > on either complier, or any use of __BF_FIELD_CHECK() will not compile > > with -O0? > > Any use of __BF_FIELD_CHECK. The code will compile but not link since > calls to ____compiletime_assert_xxx (added by compiletime_assert > macro) will not be removed in -O0. Why would that be, it's just a macro? Does it by extension mean any use of BUILD_BUG_ON_MSG() will not compile with -O0?
On Wed, Oct 4, 2017 at 7:06 PM, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > On Wed, 4 Oct 2017 18:50:04 -0700, Manoj Gupta wrote: >> On Wed, Oct 4, 2017 at 5:56 PM, Jakub Kicinski wrote: >> > On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote: >> >> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote: >> >> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote: >> >> >> > > Thanks for the suggestion. This seems a viable alternative if David >> >> >> > > and the NFP owners can live without the extra checking provided by >> >> >> > > __BF_FIELD_CHECK. >> >> >> > >> >> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks >> >> >> > is that it will require runtime ffs on the mask, which is potentially >> >> >> > costly. I would also feel quite stupid adding those macros to the nfp >> >> >> > driver, given that I specifically created the bitfield.h header to not >> >> >> > have to reimplement these in every driver I write/maintain. >> >> >> >> >> >> That make sense, thanks for providing more context. >> >> >> >> >> >> > Can you please test the patch I provided in the other reply? >> >> >> >> >> >> With this patch there are no errors when building the kernel with >> >> >> clang. >> >> > >> >> > Cool, thanks for checking! I will run it through full tests and queue >> >> > for upstreaming :) >> >> >> >> Just to let you know, using __BF_FIELD_CHECK macro will not Link with >> >> -O0 (GCC or Clang) since references to __compiletime_assert_xxx will >> >> not be cleaned up. >> > >> > Do you mean the current nfp_eth_set_bit_config() will not work with -O0 >> > on either complier, or any use of __BF_FIELD_CHECK() will not compile >> > with -O0? >> >> Any use of __BF_FIELD_CHECK. The code will compile but not link since >> calls to ____compiletime_assert_xxx (added by compiletime_assert >> macro) will not be removed in -O0. > > Why would that be, it's just a macro? Does it by extension mean any > use of BUILD_BUG_ON_MSG() will not compile with -O0? You have to look at the the code added once the macro is expanded :). Please look at implementation of compiletime_assert at http://elixir.free-electrons.com/linux/v4.12.14/source/include/linux/compiler.h#L507 It creates a call to __compiler_assert_xxx inside a loop which is not cleaned up in -O0. Thanks, Manoj
El Wed, Oct 04, 2017 at 07:13:26PM -0700 Manoj Gupta ha dit: > On Wed, Oct 4, 2017 at 7:06 PM, Jakub Kicinski > <jakub.kicinski@netronome.com> wrote: > > On Wed, 4 Oct 2017 18:50:04 -0700, Manoj Gupta wrote: > >> On Wed, Oct 4, 2017 at 5:56 PM, Jakub Kicinski wrote: > >> > On Wed, 4 Oct 2017 17:38:22 -0700, Manoj Gupta wrote: > >> >> On Wed, Oct 4, 2017 at 4:25 PM, Jakub Kicinski wrote: > >> >> > On Wed, 4 Oct 2017 16:16:49 -0700, Matthias Kaehlcke wrote: > >> >> >> > > Thanks for the suggestion. This seems a viable alternative if David > >> >> >> > > and the NFP owners can live without the extra checking provided by > >> >> >> > > __BF_FIELD_CHECK. > >> >> >> > > >> >> >> > The reason the __BF_FIELD_CHECK refuses to compile non-constant masks > >> >> >> > is that it will require runtime ffs on the mask, which is potentially > >> >> >> > costly. I would also feel quite stupid adding those macros to the nfp > >> >> >> > driver, given that I specifically created the bitfield.h header to not > >> >> >> > have to reimplement these in every driver I write/maintain. > >> >> >> > >> >> >> That make sense, thanks for providing more context. > >> >> >> > >> >> >> > Can you please test the patch I provided in the other reply? > >> >> >> > >> >> >> With this patch there are no errors when building the kernel with > >> >> >> clang. > >> >> > > >> >> > Cool, thanks for checking! I will run it through full tests and queue > >> >> > for upstreaming :) > >> >> > >> >> Just to let you know, using __BF_FIELD_CHECK macro will not Link with > >> >> -O0 (GCC or Clang) since references to __compiletime_assert_xxx will > >> >> not be cleaned up. > >> > > >> > Do you mean the current nfp_eth_set_bit_config() will not work with -O0 > >> > on either complier, or any use of __BF_FIELD_CHECK() will not compile > >> > with -O0? > >> > >> Any use of __BF_FIELD_CHECK. The code will compile but not link since > >> calls to ____compiletime_assert_xxx (added by compiletime_assert > >> macro) will not be removed in -O0. > > > > Why would that be, it's just a macro? Does it by extension mean any > > use of BUILD_BUG_ON_MSG() will not compile with -O0? > > You have to look at the the code added once the macro is expanded :). > Please look at implementation of compiletime_assert at > http://elixir.free-electrons.com/linux/v4.12.14/source/include/linux/compiler.h#L507 > It creates a call to __compiler_assert_xxx inside a loop which is not > cleaned up in -O0. I just saw that v4.14 will have a fix for that: commit c03567a8e8d5cf2aaca40e605c48f319dc2ead57 Author: Joe Stringer <joe@ovn.org> Date: Thu Aug 31 16:15:33 2017 -0700 include/linux/compiler.h: don't perform compiletime_assert with -O0 Obviously this means that the checks aren't performed, however that shouldn't be an issue since AFAIK the kernel doesn't officially support -O0 builds in the first place.
Hi Jakub, El Wed, Oct 04, 2017 at 11:17:24AM -0700 Jakub Kicinski ha dit: > On Wed, 4 Oct 2017 10:42:42 -0700, Manoj Gupta wrote: > > Hi Jakub, > > > > I had discussed about supporting this code with some clang developers. > > However, the consensus was this code relies on a specific GCC optimizer > > behavior and Clang does not share the same behavior by design. > > Hm. I find surprising that constant propagation to inlined functions > is considered something GCC-specific rather than obvious/basic. > > > Note that even GCC can't compile this code at -O0. > > Yes, that makes me feel uncomfortable... Could you try this? > > -------8<------- > > diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > index f6f7c085f8e0..47251396fcae 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > @@ -469,10 +469,10 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed) > return nfp_eth_config_commit_end(nsp); > } > > -/* Force inline, FIELD_* macroes require masks to be compilation-time known */ > -static __always_inline int > +static int > nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > - const u64 mask, unsigned int val, const u64 ctrl_bit) > + const u64 mask, const unsigned int shift, > + unsigned int val, const u64 ctrl_bit) > { > union eth_table_entry *entries = nfp_nsp_config_entries(nsp); > unsigned int idx = nfp_nsp_config_idx(nsp); > @@ -489,11 +489,11 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > > /* Check if we are already in requested state */ > reg = le64_to_cpu(entries[idx].raw[raw_idx]); > - if (val == FIELD_GET(mask, reg)) > + if (val == (reg & mask) >> shift) > return 0; > > reg &= ~mask; > - reg |= FIELD_PREP(mask, val); > + reg |= (val << shift) & mask; > entries[idx].raw[raw_idx] = cpu_to_le64(reg); > > entries[idx].control |= cpu_to_le64(ctrl_bit); > @@ -503,6 +503,13 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > return 0; > } > > +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit) \ > + ({ \ > + __BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \ > + nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \ > + val, ctrl_bit); \ > + }) > + > /** > * __nfp_eth_set_aneg() - set PHY autonegotiation control bit > * @nsp: NFP NSP handle returned from nfp_eth_config_start() > @@ -515,7 +522,7 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > */ > int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode) > { > - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE, > + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE, > NSP_ETH_STATE_ANEG, mode, > NSP_ETH_CTRL_SET_ANEG); > } > @@ -544,7 +551,7 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed) > return -EINVAL; > } > > - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE, > + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE, > NSP_ETH_STATE_RATE, rate, > NSP_ETH_CTRL_SET_RATE); > } > @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed) > */ > int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes) > { > - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, > + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, > lanes, NSP_ETH_CTRL_SET_LANES); > } Do you plan to send this as an official patch? Thanks Matthias
On Tue, 24 Oct 2017 09:56:03 -0700, Matthias Kaehlcke wrote: > > @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed) > > */ > > int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes) > > { > > - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, > > + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, > > lanes, NSP_ETH_CTRL_SET_LANES); > > } > > Do you plan to send this as an official patch? Sorry... Dirk is prepping a larger series for this area of code and it got stuck there a bit :S He says it's coming any day now...
El Tue, Oct 24, 2017 at 10:03:56AM -0700 Jakub Kicinski ha dit: > On Tue, 24 Oct 2017 09:56:03 -0700, Matthias Kaehlcke wrote: > > > @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed) > > > */ > > > int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes) > > > { > > > - return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, > > > + return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES, > > > lanes, NSP_ETH_CTRL_SET_LANES); > > > } > > > > Do you plan to send this as an official patch? > > Sorry... Dirk is prepping a larger series for this area of code and it > got stuck there a bit :S No prob, just wanted to make sure it didn't slip under the radar. > He says it's coming any day now... Great, thanks!
diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c index f6f7c085f8e0..e9c635867918 100644 --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c @@ -469,39 +469,40 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed) return nfp_eth_config_commit_end(nsp); } -/* Force inline, FIELD_* macroes require masks to be compilation-time known */ -static __always_inline int -nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, - const u64 mask, unsigned int val, const u64 ctrl_bit) -{ - union eth_table_entry *entries = nfp_nsp_config_entries(nsp); - unsigned int idx = nfp_nsp_config_idx(nsp); - u64 reg; - - /* Note: set features were added in ABI 0.14 but the error - * codes were initially not populated correctly. - */ - if (nfp_nsp_get_abi_ver_minor(nsp) < 17) { - nfp_err(nfp_nsp_cpp(nsp), - "set operations not supported, please update flash\n"); - return -EOPNOTSUPP; - } - - /* Check if we are already in requested state */ - reg = le64_to_cpu(entries[idx].raw[raw_idx]); - if (val == FIELD_GET(mask, reg)) - return 0; - - reg &= ~mask; - reg |= FIELD_PREP(mask, val); - entries[idx].raw[raw_idx] = cpu_to_le64(reg); - - entries[idx].control |= cpu_to_le64(ctrl_bit); - - nfp_nsp_config_set_modified(nsp, true); - - return 0; -} +#define nfp_eth_set_bit_config(nsp, raw_idx, mask, val, ctrl_bit) \ +({ \ + union eth_table_entry *entries = nfp_nsp_config_entries(nsp); \ + unsigned int idx = nfp_nsp_config_idx(nsp); \ + u64 reg; \ + int rc; \ + \ + /* Note: set features were added in ABI 0.14 but the error */ \ + /* codes were initially not populated correctly. */ \ + if (nfp_nsp_get_abi_ver_minor(nsp) < 17) { \ + nfp_err(nfp_nsp_cpp(nsp), \ + "set operations not supported, please update flash\n"); \ + rc = -EOPNOTSUPP; \ + goto out; \ + } \ + \ + rc = 0; \ + \ + /* Check if we are already in requested state */ \ + reg = le64_to_cpu(entries[idx].raw[raw_idx]); \ + if (val == FIELD_GET(mask, reg)) \ + goto out; \ + \ + reg &= ~mask; \ + reg |= FIELD_PREP(mask, val); \ + entries[idx].raw[raw_idx] = cpu_to_le64(reg); \ + \ + entries[idx].control |= cpu_to_le64(ctrl_bit); \ + \ + nfp_nsp_config_set_modified(nsp, true); \ + \ +out: \ + rc; \ +}) /** * __nfp_eth_set_aneg() - set PHY autonegotiation control bit
nfp_eth_set_bit_config() is marked as __always_inline to allow gcc to identify the 'mask' parameter as known to be constant at compile time, which is required to use the FIELD_GET() macro. The forced inlining does the trick for gcc, but for kernel builds with clang it results in undefined symbols: drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.o: In function `__nfp_eth_set_aneg': drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x787): undefined reference to `__compiletime_assert_492' drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c:(.text+0x7b1): undefined reference to `__compiletime_assert_496' These __compiletime_assert_xyx() calls would have been optimized away if the compiler had seen 'mask' as a constant. Convert nfp_eth_set_bit_config() into a macro, which allows both gcc and clang to identify 'mask' as a compile time constant. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- I am aware that a lengthy macro is not a pretty solution, I'm open for better suggestions. Note: The patch has been build-tested only since I don't have any NFP hardware. .../ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c | 67 +++++++++++----------- 1 file changed, 34 insertions(+), 33 deletions(-)