All of lore.kernel.org
 help / color / mirror / Atom feed
From: Joe Perches <joe@perches.com>
To: Jonathan Toppins <jtoppins@redhat.com>, netdev@vger.kernel.org
Cc: Jay Vosburgh <j.vosburgh@gmail.com>,
	Veaceslav Falico <vfalico@gmail.com>,
	Andy Gospodarek <andy@greyhouse.net>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 2/2] bonding: combine netlink and console error messages
Date: Sun, 08 Aug 2021 22:05:30 -0700	[thread overview]
Message-ID: <686044636dbf886e7eedb626ca1569e82eac1a64.camel@perches.com> (raw)
In-Reply-To: <b384c564-8467-1504-026c-5a437cad1a14@redhat.com>

On Sun, 2021-08-08 at 22:07 -0400, Jonathan Toppins wrote:
> On 8/8/21 6:02 AM, Joe Perches wrote:
> > On Sat, 2021-08-07 at 17:54 -0400, Jonathan Toppins wrote:
> > > On 8/6/21 11:52 PM, Joe Perches wrote:
> > > > On Fri, 2021-08-06 at 23:30 -0400, Jonathan Toppins wrote:
> > > > > There seems to be no reason to have different error messages between
> > > > > netlink and printk. It also cleans up the function slightly.
> > > > []
> > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > > > []
> > > > > +#define BOND_NL_ERR(bond_dev, extack, errmsg) do {		\
> > > > > +	NL_SET_ERR_MSG(extack, errmsg);				\
> > > > > +	netdev_err(bond_dev, "Error: " errmsg "\n");		\
> > > > > +} while (0)
> > > > > +
> > > > > +#define SLAVE_NL_ERR(bond_dev, slave_dev, extack, errmsg) do {	\
> > > > > +	NL_SET_ERR_MSG(extack, errmsg);				\
> > > > > +	slave_err(bond_dev, slave_dev, "Error: " errmsg "\n");	\
> > > > > +} while (0)
> > > > 
> > > > If you are doing this, it's probably smaller object code to use
> > > > 	"%s", errmsg
> > > > as the errmsg string can be reused
> > > > 
> > > > #define BOND_NL_ERR(bond_dev, extack, errmsg)			\
> > > > do {								\
> > > > 	NL_SET_ERR_MSG(extack, errmsg);				\
> > > > 	netdev_err(bond_dev, "Error: %s\n", errmsg);		\
> > > > } while (0)
> > > > 
> > > > #define SLAVE_NL_ERR(bond_dev, slave_dev, extack, errmsg)	\
> > > > do {								\
> > > > 	NL_SET_ERR_MSG(extack, errmsg);				\
> > > > 	slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);	\
> > > > } while (0)
> > > > 
> > > > 
> > > 
> > > I like the thought and would agree if not for how NL_SET_ERR_MSG is
> > > coded. Unfortunately it does not appear as though doing the above change
> > > actually generates smaller object code. Maybe I have incorrectly
> > > interpreted something?
> > 
> > No, it's because you are compiling allyesconfig or equivalent.
> > Try defconfig with bonding.
> > 
> > 
> 
> $ git clean -dxf
> $ git log -1 -p
> commit 8985f8d3fa38bca5f5384f9210ed735d58fd94f2 (HEAD -> 
> upstream-bonding-cleanup)
> Author: Jonathan Toppins <jtoppins@redhat.com>
> Date:   Sun Aug 8 21:45:14 2021 -0400
> 
>      object code optimization
> 
> diff --git a/drivers/net/bonding/bond_main.c 
> b/drivers/net/bonding/bond_main.c
> index 46b95175690b..e2903ae7cdab 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1714,12 +1714,12 @@ void bond_lower_state_changed(struct slave *slave)
> 
>   #define BOND_NL_ERR(bond_dev, extack, errmsg) do {             \
>          NL_SET_ERR_MSG(extack, errmsg);                         \
> -       netdev_err(bond_dev, "Error: " errmsg "\n");            \
> +       netdev_err(bond_dev, "Error: %s\n", errmsg);            \
>   } while (0)
> 
>   #define SLAVE_NL_ERR(bond_dev, slave_dev, extack, errmsg) do { \
>          NL_SET_ERR_MSG(extack, errmsg);                         \
> -       slave_err(bond_dev, slave_dev, "Error: " errmsg "\n");  \
> +       slave_err(bond_dev, slave_dev, "Error: %s\n", errmsg);  \
>   } while (0)
> 
>   /* enslave device <slave> to bond device <master> */
> $ git log --oneline -2
> 8985f8d3fa38 (HEAD -> upstream-bonding-cleanup) object code optimization
> e326bf8fd30f bonding: combine netlink and console error messages
> $ make defconfig
>    HOSTCC  scripts/basic/fixdep
> [...]
> *** Default configuration is based on 'x86_64_defconfig'
> #
> # configuration written to .config
> #
> $ grep "BONDING" .config
> # CONFIG_BONDING is not set
> $ make menuconfig
>    UPD     scripts/kconfig/mconf-cfg
> [...]
> configuration written to .config
> 
> *** End of the configuration.
> *** Execute 'make' to start the build or try 'make help'.
> 
> $ grep "BONDING" .config
> CONFIG_BONDING=m
> $ git rebase -i --exec "make drivers/net/bonding/bond_main.o; ls -l 
> drivers/net/bonding/bond_main.o" HEAD^^
> Executing: make drivers/net/bonding/bond_main.o; ls -l 
> drivers/net/bonding/bond_main.o
>    SYNC    include/config/auto.conf.cmd
> [...]
>    CC      /home/jtoppins/projects/linux-rhel7/tools/objtool/librbtree.o
>    LD      /home/jtoppins/projects/linux-rhel7/tools/objtool/objtool-in.o
>    LINK    /home/jtoppins/projects/linux-rhel7/tools/objtool/objtool
>    CC [M]  drivers/net/bonding/bond_main.o
> -rw-r--r--. 1 jtoppins jtoppins 131800 Aug  8 21:47 
> drivers/net/bonding/bond_main.o
> Executing: make drivers/net/bonding/bond_main.o; ls -l 
> drivers/net/bonding/bond_main.o
>    CALL    scripts/checksyscalls.sh
>    CALL    scripts/atomic/check-atomics.sh
>    DESCEND objtool
>    CC [M]  drivers/net/bonding/bond_main.o
> -rw-r--r--. 1 jtoppins jtoppins 131928 Aug  8 21:47 

Your size is significantly different than mine (x86-64 defconfig w/ bonding)

$ gcc --version
gcc (Ubuntu 10.3.0-1ubuntu1) 10.3.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Original:

$ git log -1
commit 7999516e20bd9bb5d1f7351cbd05ca529a3a8d60 (HEAD, tag: next-20210806, origin/master, origin/HEAD)
Author: Mark Brown <broonie@kernel.org>
Date:   Fri Aug 6 17:52:53 2021 +0100

    Add linux-next specific files for 20210806
    
    Signed-off-by: Mark Brown <broonie@kernel.org>

$ size drivers/net/bonding/built-in.a -t
   text	   data	    bss	    dec	    hex	filename
  59630	    399	    460	  60489	   ec49	drivers/net/bonding/bond_main.o (ex drivers/net/bonding/built-in.a)
  16790	     14	      2	  16806	   41a6	drivers/net/bonding/bond_3ad.o (ex drivers/net/bonding/built-in.a)
  17101	     50	      0	  17151	   42ff	drivers/net/bonding/bond_alb.o (ex drivers/net/bonding/built-in.a)
   7116	   1516	      0	   8632	   21b8	drivers/net/bonding/bond_sysfs.o (ex drivers/net/bonding/built-in.a)
   1411	     72	      0	   1483	    5cb	drivers/net/bonding/bond_sysfs_slave.o (ex drivers/net/bonding/built-in.a)
    165	      0	      0	    165	     a5	drivers/net/bonding/bond_debugfs.o (ex drivers/net/bonding/built-in.a)
   6971	    232	      0	   7203	   1c23	drivers/net/bonding/bond_netlink.o (ex drivers/net/bonding/built-in.a)
  15889	     74	      0	  15963	   3e5b	drivers/net/bonding/bond_options.o (ex drivers/net/bonding/built-in.a)
   4769	      0	      0	   4769	   12a1	drivers/net/bonding/bond_procfs.o (ex drivers/net/bonding/built-in.a)
 129842	   2357	    462	 132661	  20635	(TOTALS)

Then with your 2 patches:

$ size -t drivers/net/bonding/built-in.a
   text	   data	    bss	    dec	    hex	filename
  59590	    399	    460	  60449	   ec21	drivers/net/bonding/bond_main.o (ex drivers/net/bonding/built-in.a)
  16790	     14	      2	  16806	   41a6	drivers/net/bonding/bond_3ad.o (ex drivers/net/bonding/built-in.a)
  17101	     50	      0	  17151	   42ff	drivers/net/bonding/bond_alb.o (ex drivers/net/bonding/built-in.a)
   7116	   1516	      0	   8632	   21b8	drivers/net/bonding/bond_sysfs.o (ex drivers/net/bonding/built-in.a)
   1411	     72	      0	   1483	    5cb	drivers/net/bonding/bond_sysfs_slave.o (ex drivers/net/bonding/built-in.a)
    165	      0	      0	    165	     a5	drivers/net/bonding/bond_debugfs.o (ex drivers/net/bonding/built-in.a)
   6971	    232	      0	   7203	   1c23	drivers/net/bonding/bond_netlink.o (ex drivers/net/bonding/built-in.a)
  15889	     74	      0	  15963	   3e5b	drivers/net/bonding/bond_options.o (ex drivers/net/bonding/built-in.a)
   4769	      0	      0	   4769	   12a1	drivers/net/bonding/bond_procfs.o (ex drivers/net/bonding/built-in.a)
 129802	   2357	    462	 132621	  2060d	(TOTALS)

Then with my suggestion:

$ size -t drivers/net/bonding/built-in.a
   text	   data	    bss	    dec	    hex	filename
  59561	    399	    460	  60420	   ec04	drivers/net/bonding/bond_main.o (ex drivers/net/bonding/built-in.a)
  16790	     14	      2	  16806	   41a6	drivers/net/bonding/bond_3ad.o (ex drivers/net/bonding/built-in.a)
  17101	     50	      0	  17151	   42ff	drivers/net/bonding/bond_alb.o (ex drivers/net/bonding/built-in.a)
   7116	   1516	      0	   8632	   21b8	drivers/net/bonding/bond_sysfs.o (ex drivers/net/bonding/built-in.a)
   1411	     72	      0	   1483	    5cb	drivers/net/bonding/bond_sysfs_slave.o (ex drivers/net/bonding/built-in.a)
    165	      0	      0	    165	     a5	drivers/net/bonding/bond_debugfs.o (ex drivers/net/bonding/built-in.a)
   6971	    232	      0	   7203	   1c23	drivers/net/bonding/bond_netlink.o (ex drivers/net/bonding/built-in.a)
  15889	     74	      0	  15963	   3e5b	drivers/net/bonding/bond_options.o (ex drivers/net/bonding/built-in.a)
   4769	      0	      0	   4769	   12a1	drivers/net/bonding/bond_procfs.o (ex drivers/net/bonding/built-in.a)
 129773	   2357	    462	 132592	  205f0	(TOTALS)

cheers, Joe


  reply	other threads:[~2021-08-09  5:05 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-07  3:30 [PATCH net-next 0/2] bonding: cleanup header file and error msgs Jonathan Toppins
2021-08-07  3:30 ` [PATCH net-next 1/2] bonding: remove extraneous definitions from bonding.h Jonathan Toppins
2021-08-07  3:30 ` [PATCH net-next 2/2] bonding: combine netlink and console error messages Jonathan Toppins
2021-08-07  3:52   ` Joe Perches
2021-08-07 21:54     ` Jonathan Toppins
2021-08-08 10:02       ` Joe Perches
2021-08-09  2:07         ` Jonathan Toppins
2021-08-09  5:05           ` Joe Perches [this message]
2021-08-09 17:17             ` Jonathan Toppins
2021-08-08 10:16   ` Leon Romanovsky
2021-08-09  1:42     ` Jonathan Toppins
2021-08-09  6:03       ` Leon Romanovsky
2021-08-10  6:40   ` [PATCH net-next v2 " Jonathan Toppins
2021-08-10  6:47     ` Leon Romanovsky
2021-08-11  2:53 ` [PATCH net-next v2 0/2] bonding: cleanup header file and error msgs Jonathan Toppins
2021-08-11  2:53   ` [PATCH net-next v2 1/2] bonding: remove extraneous definitions from bonding.h Jonathan Toppins
2021-08-11  2:53   ` [PATCH net-next v2 2/2] bonding: combine netlink and console error messages Jonathan Toppins
2021-08-11  3:27     ` Joe Perches
2021-08-11 12:49       ` Jakub Kicinski
2021-08-11 13:23         ` Joe Perches
2021-08-11 14:31           ` Jonathan Toppins
2021-08-11 22:10   ` [PATCH net-next v2 0/2] bonding: cleanup header file and error msgs patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=686044636dbf886e7eedb626ca1569e82eac1a64.camel@perches.com \
    --to=joe@perches.com \
    --cc=andy@greyhouse.net \
    --cc=davem@davemloft.net \
    --cc=j.vosburgh@gmail.com \
    --cc=jtoppins@redhat.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=vfalico@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.