| Message ID | 1497015283-1729-1-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jun 09, 2017 at 03:34:43PM +0200, Nicolas Dichtel wrote: > Make it explicit in the log. > > Suggested-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> Hi Nicolas How often is this called? It seems like it is used by ad_port_selection_logic, which has the comment: is called in the inititalization (after all the handshkes), and after every lacpdu receive (if selected is off) I just wonder if this should be rate limited? Andrew
Le 09/06/2017 à 16:23, Andrew Lunn a écrit : > On Fri, Jun 09, 2017 at 03:34:43PM +0200, Nicolas Dichtel wrote: >> Make it explicit in the log. >> >> Suggested-by: Andrew Lunn <andrew@lunn.ch> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > Hi Nicolas Hi Andrew, > > How often is this called? It seems like it is used by Good question! ;-) > ad_port_selection_logic, which has the comment: > > is called in the inititalization (after all the handshkes), > and after every lacpdu receive (if selected is off) > > I just wonder if this should be rate limited? So using net_warn_ratelimited()? Displaying this message continuously in the log, even at a low rate, seems not the best things to do. The first time seems enough, but it would require more code for that. Is it not over-engineering? The ideal solution would be a BUILD_BUG_ON(), but I don't see how to make that. Regards, Nicolas
On Fri, Jun 09, 2017 at 04:39:06PM +0200, Nicolas Dichtel wrote: > Le 09/06/2017 à 16:23, Andrew Lunn a écrit : > > On Fri, Jun 09, 2017 at 03:34:43PM +0200, Nicolas Dichtel wrote: > >> Make it explicit in the log. > >> > >> Suggested-by: Andrew Lunn <andrew@lunn.ch> > >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > > > Hi Nicolas > Hi Andrew, > > > > > How often is this called? It seems like it is used by > Good question! ;-) > > > ad_port_selection_logic, which has the comment: > > > > is called in the inititalization (after all the handshkes), > > and after every lacpdu receive (if selected is off) > > > > I just wonder if this should be rate limited? > So using net_warn_ratelimited()? Why not just use pr_warn_once()? It may go unnoticed at first, but that seems like a decent option compared to spamming log files. > Displaying this message continuously in the log, even at a low rate, seems not > the best things to do. The first time seems enough, but it would require more > code for that. Is it not over-engineering? > The ideal solution would be a BUILD_BUG_ON(), but I don't see how to make that. >
> > I just wonder if this should be rate limited? > So using net_warn_ratelimited()? > > Displaying this message continuously in the log, even at a low rate, seems not > the best things to do. The first time seems enough, but it would require more > code for that. Is it not over-engineering? Maybe look at the _once functions, dev_warn_once, printk_once. > The ideal solution would be a BUILD_BUG_ON(), but I don't see how to make that. Me neither. Andrew
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Fri, 9 Jun 2017 15:34:43 +0200 > Make it explicit in the log. > > Suggested-by: Andrew Lunn <andrew@lunn.ch> > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> I agree with others that we should rate limit this somehow given the context in which it is invoked.
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c index b44a6aeb346d..b15b177662b0 100644 --- a/drivers/net/bonding/bond_3ad.c +++ b/drivers/net/bonding/bond_3ad.c @@ -322,6 +322,9 @@ static u16 __get_link_speed(struct port *port) default: /* unknown speed value from ethtool. shouldn't happen */ + pr_warn("%s: unknown speed (%d) for port %d (set it to 0)\n", + slave->bond->dev->name, slave->speed, + port->actor_port_number); speed = 0; break; }
Make it explicit in the log. Suggested-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- drivers/net/bonding/bond_3ad.c | 3 +++ 1 file changed, 3 insertions(+)