| Message ID | 20190801090347.8258-1-liuhangbin@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ibmveth: use net_err_ratelimited when set_multicast_list | expand |
On Thu, 2019-08-01 at 17:03 +0800, Hangbin Liu wrote: > When setting lots of multicast list on ibmveth, e.g. add 3000 membership on a > multicast group, the following error message flushes our log file > > 8507 [ 901.478251] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table > ... > 1718386 [ 5636.808658] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table > > We got 1.5 million lines of messages in 1.3h. Let's replace netdev_err() by > net_err_ratelimited() to avoid this issue. I don't use netdev_err_once() in > case h_multicast_ctrl() return different lpar_rc types. > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > drivers/net/ethernet/ibm/ibmveth.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c > index d654c234aaf7..3b9406a4ca92 100644 > --- a/drivers/net/ethernet/ibm/ibmveth.c > +++ b/drivers/net/ethernet/ibm/ibmveth.c > @@ -1446,9 +1446,11 @@ static void ibmveth_set_multicast_list(struct net_device *netdev) > IbmVethMcastAddFilter, > mcast_addr); > if (lpar_rc != H_SUCCESS) { > - netdev_err(netdev, "h_multicast_ctrl rc=%ld " > - "when adding an entry to the filter " > - "table\n", lpar_rc); > + net_err_ratelimited("%s %s%s: h_multicast_ctrl rc=%ld when adding an entry to the filter table\n", > + ibmveth_driver_name, > + netdev_name(netdev), > + netdev_reg_state(netdev), > + lpar_rc); Perhaps add the netdev_<level>_ratelimited variants and use that instead Somthing like: --- include/linux/netdevice.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 88292953aa6f..37116019e14f 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -4737,6 +4737,60 @@ do { \ #define netdev_info_once(dev, fmt, ...) \ netdev_level_once(KERN_INFO, dev, fmt, ##__VA_ARGS__) +#define netdev_level_ratelimited(netdev_level, dev, fmt, ...) \ +do { \ + static DEFINE_RATELIMIT_STATE(_rs, \ + DEFAULT_RATELIMIT_INTERVAL, \ + DEFAULT_RATELIMIT_BURST); \ + if (__ratelimit(&_rs)) \ + netdev_level(dev, fmt, ##__VA_ARGS__); \ +} while (0) + +#define netdev_emerg_ratelimited(dev, fmt, ...) \ + netdev_level_ratelimited(netdev_emerg, dev, fmt, ##__VA_ARGS__) +#define netdev_alert_ratelimited(dev, fmt, ...) \ + netdev_level_ratelimited(netdev_alert, dev, fmt, ##__VA_ARGS__) +#define netdev_crit_ratelimited(dev, fmt, ...) \ + netdev_level_ratelimited(netdev_crit, dev, fmt, ##__VA_ARGS__) +#define netdev_err_ratelimited(dev, fmt, ...) \ + netdev_level_ratelimited(netdev_err, dev, fmt, ##__VA_ARGS__) +#define netdev_warn_ratelimited(dev, fmt, ...) \ + netdev_level_ratelimited(netdev_warn, dev, fmt, ##__VA_ARGS__) +#define netdev_notice_ratelimited(dev, fmt, ...) \ + netdev_level_ratelimited(netdev_notice, dev, fmt, ##__VA_ARGS__) +#define netdev_info_ratelimited(dev, fmt, ...) \ + netdev_level_ratelimited(netdev_info, dev, fmt, ##__VA_ARGS__) + +#if defined(CONFIG_DYNAMIC_DEBUG) +/* descriptor check is first to prevent flooding with "callbacks suppressed" */ +#define netdev_dbg_ratelimited(dev, fmt, ...) \ +do { \ + static DEFINE_RATELIMIT_STATE(_rs, \ + DEFAULT_RATELIMIT_INTERVAL, \ + DEFAULT_RATELIMIT_BURST); \ + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \ + if (DYNAMIC_DEBUG_BRANCH(descriptor) && \ + __ratelimit(&_rs)) \ + __dynamic_netdev_dbg(&descriptor, dev, fmt, \ + ##__VA_ARGS__); \ +} while (0) +#elif defined(DEBUG) +#define netdev_dbg_ratelimited(dev, fmt, ...) \ +do { \ + static DEFINE_RATELIMIT_STATE(_rs, \ + DEFAULT_RATELIMIT_INTERVAL, \ + DEFAULT_RATELIMIT_BURST); \ + if (__ratelimit(&_rs)) \ + netdev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \ +} while (0) +#else +#define netdev_dbg_ratelimited(dev, fmt, ...) \ +do { \ + if (0) \ + netdev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \ +} while (0) +#endif + #define MODULE_ALIAS_NETDEV(device) \ MODULE_ALIAS("netdev-" device)
On Thu, Aug 01, 2019 at 03:28:43AM -0700, Joe Perches wrote: > Perhaps add the netdev_<level>_ratelimited variants and use that instead > > Somthing like: Yes, that looks better. Do you mind if I take your code and add your Signed-off-by info? Thanks Hangbin > > --- > include/linux/netdevice.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 88292953aa6f..37116019e14f 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -4737,6 +4737,60 @@ do { \ > #define netdev_info_once(dev, fmt, ...) \ > netdev_level_once(KERN_INFO, dev, fmt, ##__VA_ARGS__) > > +#define netdev_level_ratelimited(netdev_level, dev, fmt, ...) \ > +do { \ > + static DEFINE_RATELIMIT_STATE(_rs, \ > + DEFAULT_RATELIMIT_INTERVAL, \ > + DEFAULT_RATELIMIT_BURST); \ > + if (__ratelimit(&_rs)) \ > + netdev_level(dev, fmt, ##__VA_ARGS__); \ > +} while (0) > + > +#define netdev_emerg_ratelimited(dev, fmt, ...) \ > + netdev_level_ratelimited(netdev_emerg, dev, fmt, ##__VA_ARGS__) > +#define netdev_alert_ratelimited(dev, fmt, ...) \ > + netdev_level_ratelimited(netdev_alert, dev, fmt, ##__VA_ARGS__) > +#define netdev_crit_ratelimited(dev, fmt, ...) \ > + netdev_level_ratelimited(netdev_crit, dev, fmt, ##__VA_ARGS__) > +#define netdev_err_ratelimited(dev, fmt, ...) \ > + netdev_level_ratelimited(netdev_err, dev, fmt, ##__VA_ARGS__) > +#define netdev_warn_ratelimited(dev, fmt, ...) \ > + netdev_level_ratelimited(netdev_warn, dev, fmt, ##__VA_ARGS__) > +#define netdev_notice_ratelimited(dev, fmt, ...) \ > + netdev_level_ratelimited(netdev_notice, dev, fmt, ##__VA_ARGS__) > +#define netdev_info_ratelimited(dev, fmt, ...) \ > + netdev_level_ratelimited(netdev_info, dev, fmt, ##__VA_ARGS__) > + > +#if defined(CONFIG_DYNAMIC_DEBUG) > +/* descriptor check is first to prevent flooding with "callbacks suppressed" */ > +#define netdev_dbg_ratelimited(dev, fmt, ...) \ > +do { \ > + static DEFINE_RATELIMIT_STATE(_rs, \ > + DEFAULT_RATELIMIT_INTERVAL, \ > + DEFAULT_RATELIMIT_BURST); \ > + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \ > + if (DYNAMIC_DEBUG_BRANCH(descriptor) && \ > + __ratelimit(&_rs)) \ > + __dynamic_netdev_dbg(&descriptor, dev, fmt, \ > + ##__VA_ARGS__); \ > +} while (0) > +#elif defined(DEBUG) > +#define netdev_dbg_ratelimited(dev, fmt, ...) \ > +do { \ > + static DEFINE_RATELIMIT_STATE(_rs, \ > + DEFAULT_RATELIMIT_INTERVAL, \ > + DEFAULT_RATELIMIT_BURST); \ > + if (__ratelimit(&_rs)) \ > + netdev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \ > +} while (0) > +#else > +#define netdev_dbg_ratelimited(dev, fmt, ...) \ > +do { \ > + if (0) \ > + netdev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \ > +} while (0) > +#endif > + > #define MODULE_ALIAS_NETDEV(device) \ > MODULE_ALIAS("netdev-" device) > > >
From: Hangbin Liu <liuhangbin@gmail.com> Date: Thu, 1 Aug 2019 17:03:47 +0800 > When setting lots of multicast list on ibmveth, e.g. add 3000 membership on a > multicast group, the following error message flushes our log file > > 8507 [ 901.478251] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table > ... > 1718386 [ 5636.808658] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table > > We got 1.5 million lines of messages in 1.3h. Let's replace netdev_err() by > net_err_ratelimited() to avoid this issue. I don't use netdev_err_once() in > case h_multicast_ctrl() return different lpar_rc types. > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> Let's work on fixing what causes this problem, or adding a retry mechanism, rather than making the message less painful. What is worse is that these failures are not in any way communicated back up the callchain to show that the operation didn't complete sucessfully. This is a real mess in behavior and error handling, don't make it worse please.
On Thu, 2019-08-01 at 22:10 +0800, Hangbin Liu wrote: > On Thu, Aug 01, 2019 at 03:28:43AM -0700, Joe Perches wrote: > > Perhaps add the netdev_<level>_ratelimited variants and use that instead > > > > Somthing like: > > Yes, that looks better. Do you mind if I take your code and add your > Signed-off-by info? Well, if you test and verify all the paths, (I did not and just wrote that without testing), you could add something like "Suggested-by:". cheers, Joe > Thanks > Hangbin > > --- > > include/linux/netdevice.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 54 insertions(+) > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > index 88292953aa6f..37116019e14f 100644 > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > > @@ -4737,6 +4737,60 @@ do { \ > > #define netdev_info_once(dev, fmt, ...) \ > > netdev_level_once(KERN_INFO, dev, fmt, ##__VA_ARGS__) > > > > +#define netdev_level_ratelimited(netdev_level, dev, fmt, ...) \ > > +do { \ > > + static DEFINE_RATELIMIT_STATE(_rs, \ > > + DEFAULT_RATELIMIT_INTERVAL, \ > > + DEFAULT_RATELIMIT_BURST); \ > > + if (__ratelimit(&_rs)) \ > > + netdev_level(dev, fmt, ##__VA_ARGS__); \ > > +} while (0) > > + > > +#define netdev_emerg_ratelimited(dev, fmt, ...) \ > > + netdev_level_ratelimited(netdev_emerg, dev, fmt, ##__VA_ARGS__) > > +#define netdev_alert_ratelimited(dev, fmt, ...) \ > > + netdev_level_ratelimited(netdev_alert, dev, fmt, ##__VA_ARGS__) > > +#define netdev_crit_ratelimited(dev, fmt, ...) \ > > + netdev_level_ratelimited(netdev_crit, dev, fmt, ##__VA_ARGS__) > > +#define netdev_err_ratelimited(dev, fmt, ...) \ > > + netdev_level_ratelimited(netdev_err, dev, fmt, ##__VA_ARGS__) > > +#define netdev_warn_ratelimited(dev, fmt, ...) \ > > + netdev_level_ratelimited(netdev_warn, dev, fmt, ##__VA_ARGS__) > > +#define netdev_notice_ratelimited(dev, fmt, ...) \ > > + netdev_level_ratelimited(netdev_notice, dev, fmt, ##__VA_ARGS__) > > +#define netdev_info_ratelimited(dev, fmt, ...) \ > > + netdev_level_ratelimited(netdev_info, dev, fmt, ##__VA_ARGS__) > > + > > +#if defined(CONFIG_DYNAMIC_DEBUG) > > +/* descriptor check is first to prevent flooding with "callbacks suppressed" */ > > +#define netdev_dbg_ratelimited(dev, fmt, ...) \ > > +do { \ > > + static DEFINE_RATELIMIT_STATE(_rs, \ > > + DEFAULT_RATELIMIT_INTERVAL, \ > > + DEFAULT_RATELIMIT_BURST); \ > > + DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \ > > + if (DYNAMIC_DEBUG_BRANCH(descriptor) && \ > > + __ratelimit(&_rs)) \ > > + __dynamic_netdev_dbg(&descriptor, dev, fmt, \ > > + ##__VA_ARGS__); \ > > +} while (0) > > +#elif defined(DEBUG) > > +#define netdev_dbg_ratelimited(dev, fmt, ...) \ > > +do { \ > > + static DEFINE_RATELIMIT_STATE(_rs, \ > > + DEFAULT_RATELIMIT_INTERVAL, \ > > + DEFAULT_RATELIMIT_BURST); \ > > + if (__ratelimit(&_rs)) \ > > + netdev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \ > > +} while (0) > > +#else > > +#define netdev_dbg_ratelimited(dev, fmt, ...) \ > > +do { \ > > + if (0) \ > > + netdev_printk(KERN_DEBUG, dev, fmt, ##__VA_ARGS__); \ > > +} while (0) > > +#endif > > + > > #define MODULE_ALIAS_NETDEV(device) \ > > MODULE_ALIAS("netdev-" device) > > > > > >
On Thu, 2019-08-01 at 12:51 -0400, David Miller wrote: > From: Hangbin Liu <liuhangbin@gmail.com> > Date: Thu, 1 Aug 2019 17:03:47 +0800 > > > When setting lots of multicast list on ibmveth, e.g. add 3000 membership on a > > multicast group, the following error message flushes our log file > > > > 8507 [ 901.478251] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table > > ... > > 1718386 [ 5636.808658] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table > > > > We got 1.5 million lines of messages in 1.3h. Let's replace netdev_err() by > > net_err_ratelimited() to avoid this issue. I don't use netdev_err_once() in > > case h_multicast_ctrl() return different lpar_rc types. > > > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > Let's work on fixing what causes this problem, or adding a retry > mechanism, rather than making the message less painful. > > What is worse is that these failures are not in any way communicated > back up the callchain to show that the operation didn't complete > sucessfully. > > This is a real mess in behavior and error handling, don't make it > worse please. That looks as though it could be quite a chore. $ git grep -P '\bndo_set_rx_mode\s*=' | wc -l 326
On Thu, Aug 01, 2019 at 12:51:14PM -0400, David Miller wrote: > From: Hangbin Liu <liuhangbin@gmail.com> > Date: Thu, 1 Aug 2019 17:03:47 +0800 > > > When setting lots of multicast list on ibmveth, e.g. add 3000 membership on a > > multicast group, the following error message flushes our log file > > > > 8507 [ 901.478251] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table > > ... > > 1718386 [ 5636.808658] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table > > > > We got 1.5 million lines of messages in 1.3h. Let's replace netdev_err() by > > net_err_ratelimited() to avoid this issue. I don't use netdev_err_once() in > > case h_multicast_ctrl() return different lpar_rc types. > > > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > > Let's work on fixing what causes this problem, or adding a retry > mechanism, rather than making the message less painful. Yes, the multicast issue should also be fixed. It looks more like a driver issue as I haven't seen it on other drivers. I think these should be two different fixes. Thanks Hangbin > > What is worse is that these failures are not in any way communicated > back up the callchain to show that the operation didn't complete > sucessfully. > > This is a real mess in behavior and error handling, don't make it > worse please.
diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c index d654c234aaf7..3b9406a4ca92 100644 --- a/drivers/net/ethernet/ibm/ibmveth.c +++ b/drivers/net/ethernet/ibm/ibmveth.c @@ -1446,9 +1446,11 @@ static void ibmveth_set_multicast_list(struct net_device *netdev) IbmVethMcastAddFilter, mcast_addr); if (lpar_rc != H_SUCCESS) { - netdev_err(netdev, "h_multicast_ctrl rc=%ld " - "when adding an entry to the filter " - "table\n", lpar_rc); + net_err_ratelimited("%s %s%s: h_multicast_ctrl rc=%ld when adding an entry to the filter table\n", + ibmveth_driver_name, + netdev_name(netdev), + netdev_reg_state(netdev), + lpar_rc); } }
When setting lots of multicast list on ibmveth, e.g. add 3000 membership on a multicast group, the following error message flushes our log file 8507 [ 901.478251] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table ... 1718386 [ 5636.808658] ibmveth 30000003 env3: h_multicast_ctrl rc=4 when adding an entry to the filter table We got 1.5 million lines of messages in 1.3h. Let's replace netdev_err() by net_err_ratelimited() to avoid this issue. I don't use netdev_err_once() in case h_multicast_ctrl() return different lpar_rc types. Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- drivers/net/ethernet/ibm/ibmveth.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)