| Message ID | a36c7639a13963883f49c272ed7993c9625a712a.1628306392.git.jtoppins@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bonding: cleanup header file and error msgs | expand |
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? $ git show commit 6bb346263b4e9d008744b45af5105df309c35c1a (HEAD -> upstream-bonding-cleanup) Author: Jonathan Toppins <jtoppins@redhat.com> Date: Sat Aug 7 17:34:58 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 -3 6bb346263b4e (HEAD -> upstream-bonding-cleanup) object code optimization a36c7639a139 bonding: combine netlink and console error messages 88916c847e85 bonding: remove extraneous definitions from bonding.h jtoppins@jtoppins:~/projects/linux-rhel7$ git rebase -i --exec "make drivers/net/bonding/bond_main.o; ls -l drivers/net/bonding/bond_main.o" HEAD^^ hint: Waiting for your editor to close the file... Error detected while processing /home/jtoppins/.vim/bundle/cscope_macros.vim/plugin/cscope_macros.vim: line 42: E568: duplicate cscope database not added Press ENTER or type command to continue Executing: make menuconfig *** End of the configuration. *** Execute 'make' to start the build or try 'make help'. 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 DESCEND bpf/resolve_btfids CC [M] drivers/net/bonding/bond_main.o -rw-r--r--. 1 jtoppins jtoppins 1777896 Aug 7 17:37 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 DESCEND bpf/resolve_btfids CC [M] drivers/net/bonding/bond_main.o -rw-r--r--. 1 jtoppins jtoppins 1778320 Aug 7 17:37 drivers/net/bonding/bond_main.o Successfully rebased and updated refs/heads/upstream-bonding-cleanup. It appears the change increases bond_main.o by 424 (1778320-1777896) bytes.
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.
On Fri, Aug 06, 2021 at 11:30:55PM -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. > > Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> > --- > drivers/net/bonding/bond_main.c | 45 ++++++++++++++++++--------------- > 1 file changed, 25 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 3ba5f4871162..46b95175690b 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -1712,6 +1712,16 @@ void bond_lower_state_changed(struct slave *slave) > netdev_lower_state_changed(slave->dev, &info); > } > > +#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) I don't think that both extack messages and dmesg prints are needed. They both will be caused by the same source, and both will be seen by the caller, but duplicated. IMHO, errors that came from the netlink, should be printed with NL_SET_ERR_MSG(), other errors should use netdev_err/slave_err prints. Thanks
On 8/8/21 6:16 AM, Leon Romanovsky wrote: > On Fri, Aug 06, 2021 at 11:30:55PM -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. >> >> Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> >> --- >> drivers/net/bonding/bond_main.c | 45 ++++++++++++++++++--------------- >> 1 file changed, 25 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 3ba5f4871162..46b95175690b 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -1712,6 +1712,16 @@ void bond_lower_state_changed(struct slave *slave) >> netdev_lower_state_changed(slave->dev, &info); >> } >> >> +#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) > > I don't think that both extack messages and dmesg prints are needed. > > They both will be caused by the same source, and both will be seen by > the caller, but duplicated. > > IMHO, errors that came from the netlink, should be printed with NL_SET_ERR_MSG(), > other errors should use netdev_err/slave_err prints. > bond_enslave can be called from two places sysfs and netlink so reporting both a console message and netlink message makes sense to me. So I have to disagree in this case. I am simply making the two paths report the same error in the function so that when using sysfs the same information is reported. In the netlink case the information will be reported twice, once an an error response over netlink and once via printk. -Jon
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 drivers/net/bonding/bond_main.o Successfully rebased and updated refs/heads/upstream-bonding-cleanup. $ gcc --version gcc (GCC) 8.4.1 20200928 (Red Hat 8.4.1-1) Copyright (C) 2018 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. -Jon
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
On Sun, Aug 08, 2021 at 09:42:46PM -0400, Jonathan Toppins wrote: > On 8/8/21 6:16 AM, Leon Romanovsky wrote: > > On Fri, Aug 06, 2021 at 11:30:55PM -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. > > > > > > Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> > > > --- > > > drivers/net/bonding/bond_main.c | 45 ++++++++++++++++++--------------- > > > 1 file changed, 25 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > > > index 3ba5f4871162..46b95175690b 100644 > > > --- a/drivers/net/bonding/bond_main.c > > > +++ b/drivers/net/bonding/bond_main.c > > > @@ -1712,6 +1712,16 @@ void bond_lower_state_changed(struct slave *slave) > > > netdev_lower_state_changed(slave->dev, &info); > > > } > > > +#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) > > > > I don't think that both extack messages and dmesg prints are needed. > > > > They both will be caused by the same source, and both will be seen by > > the caller, but duplicated. > > > > IMHO, errors that came from the netlink, should be printed with NL_SET_ERR_MSG(), > > other errors should use netdev_err/slave_err prints. > > > > bond_enslave can be called from two places sysfs and netlink so reporting > both a console message and netlink message makes sense to me. So I have to > disagree in this case. I am simply making the two paths report the same > error in the function so that when using sysfs the same information is > reported. In the netlink case the information will be reported twice, once > an an error response over netlink and once via printk. There is no need to print any errors twice, just add "if (extack)" to you macros, something like that: +#define SLAVE_NL_ERR(bond_dev, slave_dev, extack, errmsg) do { * if (extack) \ + NL_SET_ERR_MSG(extack, errmsg); \ + else \ + slave_err(bond_dev, slave_dev, "Error: " errmsg "\n"); \ +} while (0) Thanks > > -Jon >
On 8/9/21 1:05 AM, Joe Perches wrote: > 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 > Humm I was just building the .o of the one compilation unit. I wonder if there is further optimization later. Will post a v2 with yours and Leon's changes later this evening. Appreciate the suggestions. -Jon
On Tue, Aug 10, 2021 at 02:40:31AM -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. > > v2: > - changed the printks to reduce object code slightly > - emit a single error message based on if netlink or sysfs is > attempting to enslave > > Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> > --- > drivers/net/bonding/bond_main.c | 49 +++++++++++++++++++-------------- > 1 file changed, 29 insertions(+), 20 deletions(-) Can you please resubmit whole series and not as a reply and put your changelog under ---? We don't want to see chengelog in final commit message. Thanks
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 3ba5f4871162..46b95175690b 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1712,6 +1712,16 @@ void bond_lower_state_changed(struct slave *slave) netdev_lower_state_changed(slave->dev, &info); } +#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) + /* enslave device <slave> to bond device <master> */ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, struct netlink_ext_ack *extack) @@ -1725,9 +1735,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, if (slave_dev->flags & IFF_MASTER && !netif_is_bond_master(slave_dev)) { - NL_SET_ERR_MSG(extack, "Device with IFF_MASTER cannot be enslaved"); - netdev_err(bond_dev, - "Error: Device with IFF_MASTER cannot be enslaved\n"); + BOND_NL_ERR(bond_dev, extack, + "Device with IFF_MASTER cannot be enslaved"); return -EPERM; } @@ -1739,15 +1748,13 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, /* already in-use? */ if (netdev_is_rx_handler_busy(slave_dev)) { - NL_SET_ERR_MSG(extack, "Device is in use and cannot be enslaved"); - slave_err(bond_dev, slave_dev, - "Error: Device is in use and cannot be enslaved\n"); + SLAVE_NL_ERR(bond_dev, slave_dev, extack, + "Device is in use and cannot be enslaved"); return -EBUSY; } if (bond_dev == slave_dev) { - NL_SET_ERR_MSG(extack, "Cannot enslave bond to itself."); - netdev_err(bond_dev, "cannot enslave bond to itself.\n"); + BOND_NL_ERR(bond_dev, extack, "Cannot enslave bond to itself."); return -EPERM; } @@ -1756,8 +1763,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, if (slave_dev->features & NETIF_F_VLAN_CHALLENGED) { slave_dbg(bond_dev, slave_dev, "is NETIF_F_VLAN_CHALLENGED\n"); if (vlan_uses_dev(bond_dev)) { - NL_SET_ERR_MSG(extack, "Can not enslave VLAN challenged device to VLAN enabled bond"); - slave_err(bond_dev, slave_dev, "Error: cannot enslave VLAN challenged slave on VLAN enabled bond\n"); + SLAVE_NL_ERR(bond_dev, slave_dev, extack, + "Can not enslave VLAN challenged device to VLAN enabled bond"); return -EPERM; } else { slave_warn(bond_dev, slave_dev, "enslaved VLAN challenged slave. Adding VLANs will be blocked as long as it is part of bond.\n"); @@ -1775,8 +1782,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, * enslaving it; the old ifenslave will not. */ if (slave_dev->flags & IFF_UP) { - NL_SET_ERR_MSG(extack, "Device can not be enslaved while up"); - slave_err(bond_dev, slave_dev, "slave is up - this may be due to an out of date ifenslave\n"); + SLAVE_NL_ERR(bond_dev, slave_dev, extack, + "Device can not be enslaved while up"); return -EPERM; } @@ -1815,17 +1822,15 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, bond_dev); } } else if (bond_dev->type != slave_dev->type) { - NL_SET_ERR_MSG(extack, "Device type is different from other slaves"); - slave_err(bond_dev, slave_dev, "ether type (%d) is different from other slaves (%d), can not enslave it\n", - slave_dev->type, bond_dev->type); + SLAVE_NL_ERR(bond_dev, slave_dev, extack, + "Device type is different from other slaves"); return -EINVAL; } if (slave_dev->type == ARPHRD_INFINIBAND && BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) { - NL_SET_ERR_MSG(extack, "Only active-backup mode is supported for infiniband slaves"); - slave_warn(bond_dev, slave_dev, "Type (%d) supports only active-backup mode\n", - slave_dev->type); + SLAVE_NL_ERR(bond_dev, slave_dev, extack, + "Only active-backup mode is supported for infiniband slaves"); res = -EOPNOTSUPP; goto err_undo_flags; } @@ -1839,8 +1844,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, bond->params.fail_over_mac = BOND_FOM_ACTIVE; slave_warn(bond_dev, slave_dev, "Setting fail_over_mac to active for active-backup mode\n"); } else { - NL_SET_ERR_MSG(extack, "Slave device does not support setting the MAC address, but fail_over_mac is not set to active"); - slave_err(bond_dev, slave_dev, "The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active\n"); + SLAVE_NL_ERR(bond_dev, slave_dev, extack, + "Slave device does not support setting the MAC address, but fail_over_mac is not set to active"); res = -EOPNOTSUPP; goto err_undo_flags; }
There seems to be no reason to have different error messages between netlink and printk. It also cleans up the function slightly. Signed-off-by: Jonathan Toppins <jtoppins@redhat.com> --- drivers/net/bonding/bond_main.c | 45 ++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 20 deletions(-)