* [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* Re: [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path
2022-07-21 16:19 [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path Davide Caratti
@ 2022-07-22 22:08 ` Jakub Kicinski
2022-07-22 22:27 ` Jamal Hadi Salim
2022-07-24 17:32 ` Cong Wang
2 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2022-07-22 22:08 UTC (permalink / raw)
To: Davide Caratti
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Eric Dumazet, Paolo Abeni, netdev
On Thu, 21 Jul 2022 18:19:22 +0200 Davide Caratti wrote:
> if (tcf_mirred_dev_dereference(m) == dev) {
> + pr_notice("tc mirred: target device %s is %s\n",
> + dev->name,
> + event == NETDEV_UNREGISTER ? "gone" : "down");
Should we only do this print only once per event as well?
There can be a large number of actions redirecting to a single device,
no point printing the warning multiple times for one down event.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path
2022-07-21 16:19 [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path Davide Caratti
2022-07-22 22:08 ` Jakub Kicinski
@ 2022-07-22 22:27 ` Jamal Hadi Salim
2022-07-24 17:32 ` Cong Wang
2 siblings, 0 replies; 4+ messages in thread
From: Jamal Hadi Salim @ 2022-07-22 22:27 UTC (permalink / raw)
To: Davide Caratti
Cc: Cong Wang, Jiri Pirko, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
You dont want to use the target device if it is operationally/admin down.
But if that happens momentarily then it comes back up - what happens then?
cheers,
jamal
On Thu, Jul 21, 2022 at 12:19 PM Davide Caratti <dcaratti@redhat.com> wrote:
>
> 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 [flat|nested] 4+ messages in thread* Re: [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path
2022-07-21 16:19 [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path Davide Caratti
2022-07-22 22:08 ` Jakub Kicinski
2022-07-22 22:27 ` Jamal Hadi Salim
@ 2022-07-24 17:32 ` Cong Wang
2 siblings, 0 replies; 4+ messages in thread
From: Cong Wang @ 2022-07-24 17:32 UTC (permalink / raw)
To: Davide Caratti
Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
On Thu, Jul 21, 2022 at 06:19:22PM +0200, Davide Caratti wrote:
> @@ -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;
> - }
I have no objection, just want to point it out users could still figure
out this drop by tracing kfree_skb(). _Maybe_ we could pass a reason to
kfree_skb_reason() too but it is definitely harder.
Thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-24 17:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-21 16:19 [PATCH net-next] net/sched: act_mirred: avoid printout in the traffic path Davide Caratti
2022-07-22 22:08 ` Jakub Kicinski
2022-07-22 22:27 ` Jamal Hadi Salim
2022-07-24 17:32 ` Cong Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).