Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code
Hi Vivien, [auto build test ERROR on net-next/master] url: https://github.com/0day-ci/linux/commits/Vivien-Didelot/net-dsa-tagger-simplification/20170531-032911 config: ia64-allmodconfig (attached as .config) compiler: ia64-linux-gcc (GCC) 6.2.0 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=ia64 All errors (new ones prefixed by >>): net/built-in.o: In function `eth_type_trans': >> (.text+0xbf5f2): undefined reference to `dsa_uses_tagged_protocol' net/built-in.o: In function `ic_close_devs': >> ipconfig.c:(.init.text+0x72c2): undefined reference to >> `dsa_uses_tagged_protocol' net/built-in.o: In function `ip_auto_config': ipconfig.c:(.init.text+0x8882): undefined reference to `dsa_uses_tagged_protocol' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code
Hi Andrew, Andrew Lunn writes: > On Tue, May 30, 2017 at 11:56:30AM -0400, Vivien Didelot wrote: >> Hi Andrew, David, >> >> David Miller writes: >> >> >>> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst) >> >>> +{ >> >>> +return !!dst->rcv; >> >>> +} >> >>> + >> >> >> >> You need to be careful here. This is in the hot path. Every frame >> >> received uses this code. And think about a distro kernel, which might >> >> have DSA enabled by default, yet is unlikely to have any switches. You >> >> are adding a function call which can be called millions of times per >> >> second >> > >> > Yeah, we really can't make this change. >> > >> > This isn't glibc where we're trying to hide the implementation of "FILE *" >> > behind accessor functions that caller can't see. We inline things when >> > performance dictates, and it does here. >> >> Thanks for the explanation, this wasn't obvious to me at all. So inline >> is mandatory here. Would a dereference like "!!dst->tag_ops->rcv" have >> an significant impact on performance? > > The additional dereference could cause a cache miss when accessing > tag_ops, which is expensive. dst will be in cache, so dst->rcv should > always be cheap. OK! That was interesting. I'm dropping the first 2 patches. Thanks, Vivien
Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code
On Tue, May 30, 2017 at 11:56:30AM -0400, Vivien Didelot wrote: > Hi Andrew, David, > > David Miller writes: > > >>> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst) > >>> +{ > >>> + return !!dst->rcv; > >>> +} > >>> + > >> > >> You need to be careful here. This is in the hot path. Every frame > >> received uses this code. And think about a distro kernel, which might > >> have DSA enabled by default, yet is unlikely to have any switches. You > >> are adding a function call which can be called millions of times per > >> second > > > > Yeah, we really can't make this change. > > > > This isn't glibc where we're trying to hide the implementation of "FILE *" > > behind accessor functions that caller can't see. We inline things when > > performance dictates, and it does here. > > Thanks for the explanation, this wasn't obvious to me at all. So inline > is mandatory here. Would a dereference like "!!dst->tag_ops->rcv" have > an significant impact on performance? The additional dereference could cause a cache miss when accessing tag_ops, which is expensive. dst will be in cache, so dst->rcv should always be cheap. Andrew
Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code
Hi Andrew, David, David Miller writes: >>> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst) >>> +{ >>> + return !!dst->rcv; >>> +} >>> + >> >> You need to be careful here. This is in the hot path. Every frame >> received uses this code. And think about a distro kernel, which might >> have DSA enabled by default, yet is unlikely to have any switches. You >> are adding a function call which can be called millions of times per >> second > > Yeah, we really can't make this change. > > This isn't glibc where we're trying to hide the implementation of "FILE *" > behind accessor functions that caller can't see. We inline things when > performance dictates, and it does here. Thanks for the explanation, this wasn't obvious to me at all. So inline is mandatory here. Would a dereference like "!!dst->tag_ops->rcv" have an significant impact on performance? Thanks, Vivien
Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code
From: Andrew Lunn Date: Tue, 30 May 2017 17:01:44 +0200 > On Tue, May 30, 2017 at 10:21:25AM -0400, Vivien Didelot wrote: >> Hide the implementation of dsa_uses_tagged_protocol in dsa.c since this >> helper will be extended to access the opaque dsa_device_ops structure. >> >> At the same time, fix the checkpatch comparison check: >> >> CHECK: Comparison to NULL could be written "dst->rcv" >> #41: FILE: net/dsa/dsa.c:32: >> +return dst->rcv != NULL; >> >> Signed-off-by: Vivien Didelot >> --- >> include/net/dsa.h | 5 + >> net/dsa/dsa.c | 5 + >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/include/net/dsa.h b/include/net/dsa.h >> index c0e567c0c824..cb5d668b265d 100644 >> --- a/include/net/dsa.h >> +++ b/include/net/dsa.h >> @@ -464,10 +464,7 @@ struct mii_bus *dsa_host_dev_to_mii_bus(struct device >> *dev); >> >> struct net_device *dsa_dev_to_net_device(struct device *dev); >> >> -static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst) >> -{ >> -return dst->rcv != NULL; >> -} >> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst); >> >> static inline bool netdev_uses_dsa(struct net_device *dev) >> { >> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c >> index 3288a80d4d6c..7a8a0358299b 100644 >> --- a/net/dsa/dsa.c >> +++ b/net/dsa/dsa.c >> @@ -27,6 +27,11 @@ >> >> #include "dsa_priv.h" >> >> +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst) >> +{ >> +return !!dst->rcv; >> +} >> + > > Hi Vivien > > You need to be careful here. This is in the hot path. Every frame > received uses this code. And think about a distro kernel, which might > have DSA enabled by default, yet is unlikely to have any switches. You > are adding a function call which can be called millions of times per > second Yeah, we really can't make this change. This isn't glibc where we're trying to hide the implementation of "FILE *" behind accessor functions that caller can't see. We inline things when performance dictates, and it does here.
Re: [PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code
On Tue, May 30, 2017 at 10:21:25AM -0400, Vivien Didelot wrote: > Hide the implementation of dsa_uses_tagged_protocol in dsa.c since this > helper will be extended to access the opaque dsa_device_ops structure. > > At the same time, fix the checkpatch comparison check: > > CHECK: Comparison to NULL could be written "dst->rcv" > #41: FILE: net/dsa/dsa.c:32: > + return dst->rcv != NULL; > > Signed-off-by: Vivien Didelot > --- > include/net/dsa.h | 5 + > net/dsa/dsa.c | 5 + > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index c0e567c0c824..cb5d668b265d 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -464,10 +464,7 @@ struct mii_bus *dsa_host_dev_to_mii_bus(struct device > *dev); > > struct net_device *dsa_dev_to_net_device(struct device *dev); > > -static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst) > -{ > - return dst->rcv != NULL; > -} > +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst); > > static inline bool netdev_uses_dsa(struct net_device *dev) > { > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index 3288a80d4d6c..7a8a0358299b 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -27,6 +27,11 @@ > > #include "dsa_priv.h" > > +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst) > +{ > + return !!dst->rcv; > +} > + Hi Vivien You need to be careful here. This is in the hot path. Every frame received uses this code. And think about a distro kernel, which might have DSA enabled by default, yet is unlikely to have any switches. You are adding a function call which can be called millions of times per second Andrew
[PATCH net-next 1/7] net: dsa: hide dsa_uses_tagged_protocol code
Hide the implementation of dsa_uses_tagged_protocol in dsa.c since this helper will be extended to access the opaque dsa_device_ops structure. At the same time, fix the checkpatch comparison check: CHECK: Comparison to NULL could be written "dst->rcv" #41: FILE: net/dsa/dsa.c:32: + return dst->rcv != NULL; Signed-off-by: Vivien Didelot --- include/net/dsa.h | 5 + net/dsa/dsa.c | 5 + 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/include/net/dsa.h b/include/net/dsa.h index c0e567c0c824..cb5d668b265d 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -464,10 +464,7 @@ struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev); struct net_device *dsa_dev_to_net_device(struct device *dev); -static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst) -{ - return dst->rcv != NULL; -} +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst); static inline bool netdev_uses_dsa(struct net_device *dev) { diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 3288a80d4d6c..7a8a0358299b 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -27,6 +27,11 @@ #include "dsa_priv.h" +bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst) +{ + return !!dst->rcv; +} + static struct sk_buff *dsa_slave_notag_xmit(struct sk_buff *skb, struct net_device *dev) { -- 2.13.0