Re: [RFC Patch net-next 5/6] net_sched: use rcu in fast path

On Tue, Sep 6, 2016 at 7:52 AM, Eric Dumazet [off-list ref] wrote:

On Thu, 2016-09-01 at 22:57 -0700, Cong Wang wrote:



Missing changelog ?

I was too lazy to write a changelog for this RFC patch, surely
it will be added when removing RFC.

Here I have no idea what you want to fix, since John already took care
all this infra.

Adding extra rcu_dereference() and rcu_read_lock() while the critical
RCU dereferences already happen in callers is not needed.

Of course it is, TX and RX both have rcu read lock held.

quoted

Signed-off-by: Cong Wang <redacted>
---
 net/sched/act_api.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 2f8db3c..fb6ff52 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -470,10 +470,14 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
              goto exec_done;
      }
      for (i = 0; i < nr_actions; i++) {
-             const struct tc_action *a = actions[i];
+             const struct tc_action *a;

+             rcu_read_lock();

But the caller already has rcu_read_lock() or rcu_read_lock_bh()

This was done in commit 25d8c0d55f241ce2 ("net: rcu-ify tcf_proto")

More precisely, it is __dev_queue_xmit() or netif_receive_skb_internal().
Not directly related to John.

The reason why I made it is to make it explicit that I take rcu_read_lock()
for tc action fast path, since it can be called recursively.

quoted

+             a = rcu_dereference(actions[i]);

Add in your .config :
CONFIG_SPARSE_RCU_POINTER=y
make C=2 M=net/sched

Yeah, kbuild-robot already reported this to me, no worries,
fixing it is already in my TODO list.

quoted

 repeat:
              ret = a->ops->act(skb, a, res);
+             rcu_read_unlock();
+
              if (ret == TC_ACT_REPEAT)
                      goto repeat;    /* we need a ttl - JHS */
              if (ret != TC_ACT_PIPE)

I do not believe this patch is necessary.

Please add John as reviewer next time.

You missed the rcu_dereference() part.