On Thu, Feb 3, 2022 at 7:10 AM Sebastian Andrzej Siewior
<bigeasy@xxxxxxxxxxxxx> wrote:
>
> On 2022-02-02 09:43:14 [-0800], Eric Dumazet wrote:
> > Maybe worth mentioning this commit will show a negative impact, for
> > network traffic
> > over loopback interface.
> >
> > My measure of the cost of local_bh_disable()/local_bh_enable() is ~6
> > nsec on one of my lab x86 hosts.
>
> So you are worried that
> dev_loopback_xmit() -> netif_rx_ni()
>
> becomes
> dev_loopback_xmit() -> netif_rx()
No, the loopback device (ifconfig log) I am referring to is in
drivers/net/loopback.c
loopback_xmit() calls netif_rx() directly, while bh are already disabled.
>
> and by that 6nsec slower because of that bh off/on? Can these 6nsec get
> a little lower if we substract the overhead of preempt-off/on?
> But maybe I picked the wrong loopback here.
>
> > Perhaps we could have a generic netif_rx(), and a __netif_rx() for the
> > virtual drivers (lo and maybe tunnels).
> >
> > void __netif_rx(struct sk_buff *skb);
> >
> > static inline int netif_rx(struct sk_buff *skb)
> > {
> > int res;
> > local_bh_disable();
> > res = __netif_rx(skb);
> > local_bh_enable();
> > return res;
> > }
>
> But what is __netif_rx() doing? netif_rx_ni() has this part:
>
> | preempt_disable();
> | err = netif_rx_internal(skb);
> | if (local_softirq_pending())
> | do_softirq();
> | preempt_enable();
>
> to ensure that smp_processor_id() and friends are quiet plus any raised
> softirqs are processed. With the current netif_rx() we end up with:
>
> | local_bh_disable();
> | ret = netif_rx_internal(skb);
> | local_bh_enable();
>
> which provides the same. Assuming __netif_rx() as:
>
> | int __netif_rx(skb)
> | {
> | trace_netif_rx_entry(skb);
> |
> | ret = netif_rx_internal(skb);
> | trace_netif_rx_exit(ret);
> |
> | return ret;
> | }
>
> and the loopback interface is not invoking this in_interrupt() context.
>
> Sebastian
Instead of adding a local_bh_disable()/local_bh_enable() in netif_rx()
I suggested
to rename current netif_rx() to __netif_rx() and add a wrapper, eg :
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e490b84732d1654bf067b30f2bb0b0825f88dea9..39232d99995cbd54c74e85905bb4af43b5b301ca
100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3668,7 +3668,17 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff
*skb, struct xdp_buff *xdp,
struct bpf_prog *xdp_prog);
void generic_xdp_tx(struct sk_buff *skb, struct bpf_prog *xdp_prog);
int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb);
-int netif_rx(struct sk_buff *skb);
+int __netif_rx(struct sk_buff *skb);
+static inline int netif_rx(struct sk_buff *skb)
+{
+ int res;
+
+ local_bh_disable();
+ res = __netif_rx(skb);
+ local_bh_enable();
+ return res;
+}
+
int netif_rx_ni(struct sk_buff *skb);
int netif_rx_any_context(struct sk_buff *skb);
int netif_receive_skb(struct sk_buff *skb);
diff --git a/net/core/dev.c b/net/core/dev.c
index 1baab07820f65f9bcf88a6d73e2c9ff741d33c18..f962e549e0bfea96cdba5bc7e1d8694e46652eac
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4819,7 +4819,7 @@ static int netif_rx_internal(struct sk_buff *skb)
}
/**
- * netif_rx - post buffer to the network code
+ * __netif_rx - post buffer to the network code
* @skb: buffer to post
*
* This function receives a packet from a device driver and queues it for
@@ -4833,7 +4833,7 @@ static int netif_rx_internal(struct sk_buff *skb)
*
*/
-int netif_rx(struct sk_buff *skb)
+int __netif_rx(struct sk_buff *skb)
{
int ret;
@@ -4844,7 +4844,7 @@ int netif_rx(struct sk_buff *skb)
return ret;
}
-EXPORT_SYMBOL(netif_rx);
+EXPORT_SYMBOL(__netif_rx);
int netif_rx_ni(struct sk_buff *skb)
{
