Question on 5.4.55 merge into 5.4-rt

Question on 5.4.55 merge into 5.4-rt

[Steven Rostedt]

When merging 5.4.55 into 5.4-rt I hit the following conflict:

static void flush_backlog(struct work_struct *work)
{
	struct sk_buff *skb, *tmp;
	struct softnet_data *sd;

	local_bh_disable();
	sd = this_cpu_ptr(&softnet_data);

	local_irq_disable();
	rps_lock(sd);
	skb_queue_walk_safe(&sd->input_pkt_queue, skb, tmp) {
		if (skb->dev->reg_state == NETREG_UNREGISTERING) {
			__skb_unlink(skb, &sd->input_pkt_queue);
<<<<<<< HEAD
			__skb_queue_tail(&sd->tofree_queue, skb);
=======
			dev_kfree_skb_irq(skb);
>>>>>>> v5.4.55
			input_queue_head_incr(sd);
		}
	}

The diff of 5.4.54 -> 5.4.55 of this code is:

--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5229,7 +5229,7 @@ static void flush_backlog(struct work_struct *work)
        skb_queue_walk_safe(&sd->input_pkt_queue, skb, tmp) {
                if (skb->dev->reg_state == NETREG_UNREGISTERING) {
                        __skb_unlink(skb, &sd->input_pkt_queue);
-                       kfree_skb(skb);
+                       dev_kfree_skb_irq(skb);
                        input_queue_head_incr(sd);
                }
        }


From upstream commit:

7df5cb75cfb8a ("dev: Defer free of skbs in flush_backlog")

According to that commit, it looks like kfree_skb() shouldn't be called
with irqs disabled (yeah for RT!). It now calls dev_kfree_skb_irq()
which puts the skb on the softnet_data.completion_queue, and raises the
NET_TX_SOFTIRQ to do the freeing.


This is similar to what v5.4-rt does, which a diff of 5.4.54 -> v5.4-rt:

@@ -5229,7 +5234,7 @@ static void flush_backlog(struct work_struct *work)
        skb_queue_walk_safe(&sd->input_pkt_queue, skb, tmp) {
                if (skb->dev->reg_state == NETREG_UNREGISTERING) {
                        __skb_unlink(skb, &sd->input_pkt_queue);
-                       kfree_skb(skb);
+                       __skb_queue_tail(&sd->tofree_queue, skb);
                        input_queue_head_incr(sd);
                }
        }
@@ -5239,11 +5244,14 @@ static void flush_backlog(struct work_struct *work)
        skb_queue_walk_safe(&sd->process_queue, skb, tmp) {
                if (skb->dev->reg_state == NETREG_UNREGISTERING) {
                        __skb_unlink(skb, &sd->process_queue);
-                       kfree_skb(skb);
+                       __skb_queue_tail(&sd->tofree_queue, skb);
                        input_queue_head_incr(sd);
                }
        }
+       if (!skb_queue_empty(&sd->tofree_queue))
+               raise_softirq_irqoff(NET_RX_SOFTIRQ);
        local_bh_enable();
+
 }


Where we are doing something slightly different. Placing the skb on the
sd->tofree_queue and raising NET_RX_SOFTIQ instead.

Now that the vanilla stable 5.4 kernel doesn't call kfree_skb() from
irqs_disabled, can I safely revert this entire change?

Is it safe to call kfree_skb() from local_bh_disable()?

I'm assuming it is, but just want to clarify. I'll be continuing
merging latest stable (with this revert), but please yell if you think
it will break?

Thanks!

-- Steve

Re: Question on 5.4.55 merge into 5.4-rt

[Sebastian Andrzej Siewior]

On 2020-08-14 18:54:21 [-0400], Steven Rostedt wrote:
> 
> When merging 5.4.55 into 5.4-rt I hit the following conflict:
…
> 
> Where we are doing something slightly different. Placing the skb on the
> sd->tofree_queue and raising NET_RX_SOFTIQ instead.
> 
> Now that the vanilla stable 5.4 kernel doesn't call kfree_skb() from
> irqs_disabled, can I safely revert this entire change?

Not if you mean dropping skbufhead-raw-lock.patch.

We can drop `tofree_queue' and everything related to it. We need to
keep the `raw_lock' and the `rps_lock()' hunks for
`sd->input_pkt_queue'. The other queue, `sd->process_queue', is
protected by local_bh_disable() so these hunks can be dropped in the
more recent RT versions with the re-written softirq code
(v5.0.19-rt10+).

> Is it safe to call kfree_skb() from local_bh_disable()?

of course it is.

> I'm assuming it is, but just want to clarify. I'll be continuing
> merging latest stable (with this revert), but please yell if you think
> it will break?
>
> Thanks!
> 
> -- Steve

Sebastian

Re: Question on 5.4.55 merge into 5.4-rt

[Steven Rostedt]

On Mon, 17 Aug 2020 15:41:09 +0200
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> On 2020-08-14 18:54:21 [-0400], Steven Rostedt wrote:
> > 
> > When merging 5.4.55 into 5.4-rt I hit the following conflict:  
> …
> > 
> > Where we are doing something slightly different. Placing the skb on the
> > sd->tofree_queue and raising NET_RX_SOFTIQ instead.
> > 
> > Now that the vanilla stable 5.4 kernel doesn't call kfree_skb() from
> > irqs_disabled, can I safely revert this entire change?  
> 
> Not if you mean dropping skbufhead-raw-lock.patch.

Yeah, I realized I worded that incorrectly. No, I meant only reverting
the portion of that patch I showed:

@@ -5229,7 +5234,7 @@ static void flush_backlog(struct work_struct *work)
        skb_queue_walk_safe(&sd->input_pkt_queue, skb, tmp) {
                if (skb->dev->reg_state == NETREG_UNREGISTERING) {
                        __skb_unlink(skb, &sd->input_pkt_queue);
-                       kfree_skb(skb);
+                       __skb_queue_tail(&sd->tofree_queue, skb);
                        input_queue_head_incr(sd);
                }
        }
@@ -5239,11 +5244,14 @@ static void flush_backlog(struct work_struct *work)
        skb_queue_walk_safe(&sd->process_queue, skb, tmp) {
                if (skb->dev->reg_state == NETREG_UNREGISTERING) {
                        __skb_unlink(skb, &sd->process_queue);
-                       kfree_skb(skb);
+                       __skb_queue_tail(&sd->tofree_queue, skb);
                        input_queue_head_incr(sd);
                }
        }
+       if (!skb_queue_empty(&sd->tofree_queue))
+               raise_softirq_irqoff(NET_RX_SOFTIRQ);
        local_bh_enable();
+
 }



> 
> We can drop `tofree_queue' and everything related to it. We need to
> keep the `raw_lock' and the `rps_lock()' hunks for
> `sd->input_pkt_queue'. The other queue, `sd->process_queue', is
> protected by local_bh_disable() so these hunks can be dropped in the
> more recent RT versions with the re-written softirq code
> (v5.0.19-rt10+).
> 
> > Is it safe to call kfree_skb() from local_bh_disable()?  
> 
> of course it is.

Then all looks good.

Thanks, I'll push this out to the repos today.

-- Steve