[PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path

* [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path
@ 2022-07-21 16:19 Davide Caratti
  2022-07-22 22:08 ` Jakub Kicinski
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Davide Caratti @ 2022-07-21 16:19 UTC (permalink / raw)
  To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev

when tc-mirred outputs to a device that's not up, dmesg is cluttered with
messages like:

 tc mirred to Houston: device br-int is down

we can't completely remove this printout: users might be relying on it to
detect setups where tc-mirred drops everything, as discussed earlier [1].
however, we can at least reduce the amount of these messages, and improve
their content as follows:
 - add a pr_notice(...) in the .init() function, to warn users of missing
   IFF_UP flag on the target of a newly added tc-mirred action
 - check for NETDEV_DOWN in the .notifier_call() function, and add proper
   pr_notice(...) to warn users of missing/down target devices

[1] https://lore.kernel.org/netdev/CAM_iQpUvn+ijyZtLmca3n+nZmHY9cMmPYwZMp5BTv10bLUhg3Q@mail.gmail.com/

Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/sched/act_mirred.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index a1d70cf86843..4af6073e472b 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -178,6 +178,13 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 			err = -ENODEV;
 			goto put_chain;
 		}
+		if (!(ndev->flags & IFF_UP))
+			pr_notice("tc mirred: action %i %s on %s while device is down",
+				  m->tcf_index,
+				  tcf_mirred_is_act_redirect(parm->eaction) ?
+					"redirects" : "mirrors",
+				  ndev->name);
+
 		mac_header_xmit = dev_is_mac_header_xmit(ndev);
 		odev = rcu_replace_pointer(m->tcfm_dev, ndev,
 					  lockdep_is_held(&m->tcf_lock));
@@ -251,16 +258,8 @@ static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
 	m_eaction = READ_ONCE(m->tcfm_eaction);
 	retval = READ_ONCE(m->tcf_action);
 	dev = rcu_dereference_bh(m->tcfm_dev);
-	if (unlikely(!dev)) {
-		pr_notice_once("tc mirred: target device is gone\n");
+	if (unlikely(!dev || !(dev->flags & IFF_UP)))
 		goto out;
-	}
-
-	if (unlikely(!(dev->flags & IFF_UP))) {
-		net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
-				       dev->name);
-		goto out;
-	}
 
 	/* we could easily avoid the clone only if called by ingress and clsact;
 	 * since we can't easily detect the clsact caller, skip clone only for
@@ -397,16 +396,21 @@ static int mirred_device_event(struct notifier_block *unused,
 	struct tcf_mirred *m;
 
 	ASSERT_RTNL();
-	if (event == NETDEV_UNREGISTER) {
+	if (event == NETDEV_UNREGISTER || event == NETDEV_DOWN) {
 		spin_lock(&mirred_list_lock);
 		list_for_each_entry(m, &mirred_list, tcfm_list) {
 			spin_lock_bh(&m->tcf_lock);
 			if (tcf_mirred_dev_dereference(m) == dev) {
-				netdev_put(dev, &m->tcfm_dev_tracker);
-				/* Note : no rcu grace period necessary, as
-				 * net_device are already rcu protected.
-				 */
-				RCU_INIT_POINTER(m->tcfm_dev, NULL);
+				pr_notice("tc mirred: target device %s is %s\n",
+					  dev->name,
+					  event == NETDEV_UNREGISTER ? "gone" : "down");
+				if (event == NETDEV_UNREGISTER) {
+					netdev_put(dev, &m->tcfm_dev_tracker);
+					/* Note : no rcu grace period necessary, as
+					 * net_device are already rcu protected.
+					 */
+					RCU_INIT_POINTER(m->tcfm_dev, NULL);
+				}
 			}
 			spin_unlock_bh(&m->tcf_lock);
 		}
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 4+ messages in thread