Re: [RFC PATCH net-next] packet: fix using smp_processor_id() in preemptible code

Re: [RFC PATCH net-next] packet: fix using smp_processor_id() in preemptible code

[Daniel Borkmann]

On 12/12/2013 07:10 AM, Li Zhong wrote:
> This patches tries to fix the following warning on next-1211 by
> replacing smp_processor_id() with raw_smp_processor_id().

I agree with that part. Please send a fix just replacing smp_processor_id()
into raw_smp_processor_id() for net-next to netdev directly.

> Also, it seems that we could move skb_set_queue_mapping() into
> packet_pick_tx_queue(), so we avoid calling it one more time
> unnecessarily if we are going into the normal dev_queue_xmit() code
> path.

I don't agree with that part, I think this can be also beneficiary for
packets without direct xmit, as in PF_PACKET we don't have a notion of
"flow" but just raw packets instead, and can keep the mapping local
depending on the current CPU as we do queue setting elsewhere in the
stack just as well.

> [   11.120893] BUG: using smp_processor_id() in preemptible [00000000] code: arping/3510
> [   11.120913] caller is .packet_sendmsg+0xc14/0xe68
> [   11.120920] CPU: 13 PID: 3510 Comm: arping Not tainted 3.13.0-rc3-next-20131211-dirty #1
> [   11.120926] Call Trace:
> [   11.120932] [c0000001f803f6f0] [c0000000000138dc] .show_stack+0x110/0x25c (unreliable)
> [   11.120942] [c0000001f803f7e0] [c00000000083dd24] .dump_stack+0xa0/0x37c
> [   11.120951] [c0000001f803f870] [c000000000493fd4] .debug_smp_processor_id+0xfc/0x12c
> [   11.120959] [c0000001f803f900] [c0000000007eba78] .packet_sendmsg+0xc14/0xe68
> [   11.120968] [c0000001f803fa80] [c000000000700968] .sock_sendmsg+0xa0/0xe0
> [   11.120975] [c0000001f803fbf0] [c0000000007014d8] .SyS_sendto+0x100/0x148
> [   11.120983] [c0000001f803fd60] [c0000000006fff10] .SyS_socketcall+0x1c4/0x2e8
> [   11.120990] [c0000001f803fe30] [c00000000000a1e4] syscall_exit+0x0/0x9c
>
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> ---
>   net/packet/af_packet.c |   16 ++++++++--------
>   1 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 9d70f13..20f6d56 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -237,6 +237,13 @@ struct packet_skb_cb {
>   static void __fanout_unlink(struct sock *sk, struct packet_sock *po);
>   static void __fanout_link(struct sock *sk, struct packet_sock *po);
>
> +static u16 packet_pick_tx_queue(struct net_device *dev, struct sk_buff *skb)
> +{
> +	u16 queue_index = (u16)raw_smp_processor_id() % dev->real_num_tx_queues;
> +	skb_set_queue_mapping(skb, queue_index);
> +	return queue_index;
> +}
> +
>   static int packet_direct_xmit(struct sk_buff *skb)
>   {
>   	struct net_device *dev = skb->dev;
> @@ -259,7 +266,7 @@ static int packet_direct_xmit(struct sk_buff *skb)
>   		return NET_XMIT_DROP;
>   	}
>
> -	queue_map = skb_get_queue_mapping(skb);
> +	queue_map = packet_pick_tx_queue(dev, skb);
>   	txq = netdev_get_tx_queue(dev, queue_map);
>
>   	__netif_tx_lock_bh(txq);
> @@ -308,11 +315,6 @@ static bool packet_use_direct_xmit(const struct packet_sock *po)
>   	return po->xmit == packet_direct_xmit;
>   }
>
> -static u16 packet_pick_tx_queue(struct net_device *dev)
> -{
> -	return (u16) smp_processor_id() % dev->real_num_tx_queues;
> -}
> -
>   /* register_prot_hook must be invoked with the po->bind_lock held,
>    * or from a context in which asynchronous accesses to the packet
>    * socket is not possible (packet_create()).
> @@ -2219,7 +2221,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>   			}
>   		}
>
> -		skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
>   		skb->destructor = tpacket_destruct_skb;
>   		__packet_set_status(po, ph, TP_STATUS_SENDING);
>   		atomic_inc(&po->tx_ring.pending);
> @@ -2429,7 +2430,6 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>   	skb->dev = dev;
>   	skb->priority = sk->sk_priority;
>   	skb->mark = sk->sk_mark;
> -	skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
>
>   	if (po->has_vnet_hdr) {
>   		if (vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
>
>

Re: [RFC PATCH net-next] packet: fix using smp_processor_id() in preemptible code

[David Miller]

Please send networking patches to netdev@vger.kernel.org, otherwise they
will get lost.

[RFC PATCH net-next] packet: fix using smp_processor_id() in preemptible code

[Li Zhong]

This patches tries to fix the following warning on next-1211 by
replacing smp_processor_id() with raw_smp_processor_id(). 

Also, it seems that we could move skb_set_queue_mapping() into
packet_pick_tx_queue(), so we avoid calling it one more time
unnecessarily if we are going into the normal dev_queue_xmit() code
path. 

[   11.120893] BUG: using smp_processor_id() in preemptible [00000000] code: arping/3510
[   11.120913] caller is .packet_sendmsg+0xc14/0xe68
[   11.120920] CPU: 13 PID: 3510 Comm: arping Not tainted 3.13.0-rc3-next-20131211-dirty #1
[   11.120926] Call Trace:
[   11.120932] [c0000001f803f6f0] [c0000000000138dc] .show_stack+0x110/0x25c (unreliable)
[   11.120942] [c0000001f803f7e0] [c00000000083dd24] .dump_stack+0xa0/0x37c
[   11.120951] [c0000001f803f870] [c000000000493fd4] .debug_smp_processor_id+0xfc/0x12c
[   11.120959] [c0000001f803f900] [c0000000007eba78] .packet_sendmsg+0xc14/0xe68
[   11.120968] [c0000001f803fa80] [c000000000700968] .sock_sendmsg+0xa0/0xe0
[   11.120975] [c0000001f803fbf0] [c0000000007014d8] .SyS_sendto+0x100/0x148
[   11.120983] [c0000001f803fd60] [c0000000006fff10] .SyS_socketcall+0x1c4/0x2e8
[   11.120990] [c0000001f803fe30] [c00000000000a1e4] syscall_exit+0x0/0x9c

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 net/packet/af_packet.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 9d70f13..20f6d56 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -237,6 +237,13 @@ struct packet_skb_cb {
 static void __fanout_unlink(struct sock *sk, struct packet_sock *po);
 static void __fanout_link(struct sock *sk, struct packet_sock *po);
 
+static u16 packet_pick_tx_queue(struct net_device *dev, struct sk_buff *skb)
+{
+	u16 queue_index = (u16)raw_smp_processor_id() % dev->real_num_tx_queues;
+	skb_set_queue_mapping(skb, queue_index);
+	return queue_index;
+}
+
 static int packet_direct_xmit(struct sk_buff *skb)
 {
 	struct net_device *dev = skb->dev;
@@ -259,7 +266,7 @@ static int packet_direct_xmit(struct sk_buff *skb)
 		return NET_XMIT_DROP;
 	}
 
-	queue_map = skb_get_queue_mapping(skb);
+	queue_map = packet_pick_tx_queue(dev, skb);
 	txq = netdev_get_tx_queue(dev, queue_map);
 
 	__netif_tx_lock_bh(txq);
@@ -308,11 +315,6 @@ static bool packet_use_direct_xmit(const struct packet_sock *po)
 	return po->xmit == packet_direct_xmit;
 }
 
-static u16 packet_pick_tx_queue(struct net_device *dev)
-{
-	return (u16) smp_processor_id() % dev->real_num_tx_queues;
-}
-
 /* register_prot_hook must be invoked with the po->bind_lock held,
  * or from a context in which asynchronous accesses to the packet
  * socket is not possible (packet_create()).
@@ -2219,7 +2221,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 			}
 		}
 
-		skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
 		skb->destructor = tpacket_destruct_skb;
 		__packet_set_status(po, ph, TP_STATUS_SENDING);
 		atomic_inc(&po->tx_ring.pending);
@@ -2429,7 +2430,6 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 	skb->dev = dev;
 	skb->priority = sk->sk_priority;
 	skb->mark = sk->sk_mark;
-	skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
 
 	if (po->has_vnet_hdr) {
 		if (vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {

Re: [RFC PATCH net-next] packet: fix using smp_processor_id() in preemptible code

[Jesper Dangaard Brouer]

On Fri, 13 Dec 2013 07:55:03 +0800 Li Zhong <zhong@linux.vnet.ibm.com> wrote:
> On Wed, 2013-12-11 at 10:55 +0100, Daniel Borkmann wrote:
> > On 12/12/2013 07:10 AM, Li Zhong wrote:
[...]
> > > Also, it seems that we could move skb_set_queue_mapping() into
> > > packet_pick_tx_queue(), so we avoid calling it one more time
> > > unnecessarily if we are going into the normal dev_queue_xmit() code
> > > path.
> > 
> > I don't agree with that part, I think this can be also beneficiary for
> > packets without direct xmit, as in PF_PACKET we don't have a notion of
> > "flow" but just raw packets instead, and can keep the mapping local
> > depending on the current CPU as we do queue setting elsewhere in the
> > stack just as well.
> 
> It seems to me that the newly added xmit in packet_sock is
> dev_queue_xmit() by default, and in this default case, dev_queue_xmit()
> would call netdev_pick_tx(), which would set the skb queue_mapping again
> to override the value based on the current CPU. 

Yes, I think you are right, that is also my experience with the code path.

> Or did I miss something here? 

A bit related; One thing I'm missing to understand, is why the
RAW/PF_PACKET sockets have a NULL in skb->sk when they reach
__netdev_pick_tx() ? (resulting in they cannot store/cache the queue in
sk_tx_queue_set)

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [RFC PATCH net-next] packet: fix using smp_processor_id() in preemptible code

[Li Zhong]

On Wed, 2013-12-11 at 16:49 -0500, David Miller wrote:
> Please send networking patches to netdev@vger.kernel.org, otherwise they
> will get lost.
> 
Thank you for the reminder :)

Zhong

Re: [RFC PATCH net-next] packet: fix using smp_processor_id() in preemptible code

[Li Zhong]

On Wed, 2013-12-11 at 10:55 +0100, Daniel Borkmann wrote:
> On 12/12/2013 07:10 AM, Li Zhong wrote:
> > This patches tries to fix the following warning on next-1211 by
> > replacing smp_processor_id() with raw_smp_processor_id().
> 
> I agree with that part. Please send a fix just replacing smp_processor_id()
> into raw_smp_processor_id() for net-next to netdev directly.

Thank you for the review :), patch sent as you suggested. 

> 
> > Also, it seems that we could move skb_set_queue_mapping() into
> > packet_pick_tx_queue(), so we avoid calling it one more time
> > unnecessarily if we are going into the normal dev_queue_xmit() code
> > path.
> 
> I don't agree with that part, I think this can be also beneficiary for
> packets without direct xmit, as in PF_PACKET we don't have a notion of
> "flow" but just raw packets instead, and can keep the mapping local
> depending on the current CPU as we do queue setting elsewhere in the
> stack just as well.

It seems to me that the newly added xmit in packet_sock is
dev_queue_xmit() by default, and in this default case, dev_queue_xmit()
would call netdev_pick_tx(), which would set the skb queue_mapping again
to override the value based on the current CPU. 

Or did I miss something here? 

Thanks, Zhong

> 
> > [   11.120893] BUG: using smp_processor_id() in preemptible [00000000] code: arping/3510
> > [   11.120913] caller is .packet_sendmsg+0xc14/0xe68
> > [   11.120920] CPU: 13 PID: 3510 Comm: arping Not tainted 3.13.0-rc3-next-20131211-dirty #1
> > [   11.120926] Call Trace:
> > [   11.120932] [c0000001f803f6f0] [c0000000000138dc] .show_stack+0x110/0x25c (unreliable)
> > [   11.120942] [c0000001f803f7e0] [c00000000083dd24] .dump_stack+0xa0/0x37c
> > [   11.120951] [c0000001f803f870] [c000000000493fd4] .debug_smp_processor_id+0xfc/0x12c
> > [   11.120959] [c0000001f803f900] [c0000000007eba78] .packet_sendmsg+0xc14/0xe68
> > [   11.120968] [c0000001f803fa80] [c000000000700968] .sock_sendmsg+0xa0/0xe0
> > [   11.120975] [c0000001f803fbf0] [c0000000007014d8] .SyS_sendto+0x100/0x148
> > [   11.120983] [c0000001f803fd60] [c0000000006fff10] .SyS_socketcall+0x1c4/0x2e8
> > [   11.120990] [c0000001f803fe30] [c00000000000a1e4] syscall_exit+0x0/0x9c
> >
> > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> > ---
> >   net/packet/af_packet.c |   16 ++++++++--------
> >   1 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 9d70f13..20f6d56 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -237,6 +237,13 @@ struct packet_skb_cb {
> >   static void __fanout_unlink(struct sock *sk, struct packet_sock *po);
> >   static void __fanout_link(struct sock *sk, struct packet_sock *po);
> >
> > +static u16 packet_pick_tx_queue(struct net_device *dev, struct sk_buff *skb)
> > +{
> > +	u16 queue_index = (u16)raw_smp_processor_id() % dev->real_num_tx_queues;
> > +	skb_set_queue_mapping(skb, queue_index);
> > +	return queue_index;
> > +}
> > +
> >   static int packet_direct_xmit(struct sk_buff *skb)
> >   {
> >   	struct net_device *dev = skb->dev;
> > @@ -259,7 +266,7 @@ static int packet_direct_xmit(struct sk_buff *skb)
> >   		return NET_XMIT_DROP;
> >   	}
> >
> > -	queue_map = skb_get_queue_mapping(skb);
> > +	queue_map = packet_pick_tx_queue(dev, skb);
> >   	txq = netdev_get_tx_queue(dev, queue_map);
> >
> >   	__netif_tx_lock_bh(txq);
> > @@ -308,11 +315,6 @@ static bool packet_use_direct_xmit(const struct packet_sock *po)
> >   	return po->xmit == packet_direct_xmit;
> >   }
> >
> > -static u16 packet_pick_tx_queue(struct net_device *dev)
> > -{
> > -	return (u16) smp_processor_id() % dev->real_num_tx_queues;
> > -}
> > -
> >   /* register_prot_hook must be invoked with the po->bind_lock held,
> >    * or from a context in which asynchronous accesses to the packet
> >    * socket is not possible (packet_create()).
> > @@ -2219,7 +2221,6 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >   			}
> >   		}
> >
> > -		skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
> >   		skb->destructor = tpacket_destruct_skb;
> >   		__packet_set_status(po, ph, TP_STATUS_SENDING);
> >   		atomic_inc(&po->tx_ring.pending);
> > @@ -2429,7 +2430,6 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> >   	skb->dev = dev;
> >   	skb->priority = sk->sk_priority;
> >   	skb->mark = sk->sk_mark;
> > -	skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
> >
> >   	if (po->has_vnet_hdr) {
> >   		if (vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> >
> >
> 

Re: [RFC PATCH net-next] packet: fix using smp_processor_id() in preemptible code

[Li Zhong]

On Thu, 2013-12-12 at 08:18 +0100, Jesper Dangaard Brouer wrote:
> On Fri, 13 Dec 2013 07:55:03 +0800 Li Zhong <zhong@linux.vnet.ibm.com> wrote:
> > On Wed, 2013-12-11 at 10:55 +0100, Daniel Borkmann wrote:
> > > On 12/12/2013 07:10 AM, Li Zhong wrote:
> [...]
> > > > Also, it seems that we could move skb_set_queue_mapping() into
> > > > packet_pick_tx_queue(), so we avoid calling it one more time
> > > > unnecessarily if we are going into the normal dev_queue_xmit() code
> > > > path.
> > > 
> > > I don't agree with that part, I think this can be also beneficiary for
> > > packets without direct xmit, as in PF_PACKET we don't have a notion of
> > > "flow" but just raw packets instead, and can keep the mapping local
> > > depending on the current CPU as we do queue setting elsewhere in the
> > > stack just as well.
> > 
> > It seems to me that the newly added xmit in packet_sock is
> > dev_queue_xmit() by default, and in this default case, dev_queue_xmit()
> > would call netdev_pick_tx(), which would set the skb queue_mapping again
> > to override the value based on the current CPU. 
> 
> Yes, I think you are right, that is also my experience with the code path.
> 
> > Or did I miss something here? 
> 
> A bit related; One thing I'm missing to understand, is why the
> RAW/PF_PACKET sockets have a NULL in skb->sk when they reach
> __netdev_pick_tx() ? (resulting in they cannot store/cache the queue in
> sk_tx_queue_set)

I checked the code, it seems skb->sk is not set in this code path (for
tcp, seems tcp_transmit_skb() sets it). 

Do you think we could set it here where skb_set_queue_mapping() is used,
so for this code path, we could also have it cached(but for devices
which define their queue selection method, it will not take effect)?
something like below: 

@@ -2219,7 +2219,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 			}
 		}
 
-		skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
+		skb->sk = (struct sock *)po;
 		skb->destructor = tpacket_destruct_skb;
 		__packet_set_status(po, ph, TP_STATUS_SENDING);
 		atomic_inc(&po->tx_ring.pending);
@@ -2429,7 +2429,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 	skb->dev = dev;
 	skb->priority = sk->sk_priority;
 	skb->mark = sk->sk_mark;
-	skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
+	skb->sk = sk;
 
 	if (po->has_vnet_hdr) {
 		if (vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {

Re: [RFC PATCH net-next] packet: fix using smp_processor_id() in preemptible code

[Li Zhong]

On Fri, 2013-12-13 at 14:41 +0800, Li Zhong wrote:
> On Thu, 2013-12-12 at 08:18 +0100, Jesper Dangaard Brouer wrote:
> > On Fri, 13 Dec 2013 07:55:03 +0800 Li Zhong <zhong@linux.vnet.ibm.com> wrote:
> > > On Wed, 2013-12-11 at 10:55 +0100, Daniel Borkmann wrote:
> > > > On 12/12/2013 07:10 AM, Li Zhong wrote:
> > [...]
> > > > > Also, it seems that we could move skb_set_queue_mapping() into
> > > > > packet_pick_tx_queue(), so we avoid calling it one more time
> > > > > unnecessarily if we are going into the normal dev_queue_xmit() code
> > > > > path.
> > > > 
> > > > I don't agree with that part, I think this can be also beneficiary for
> > > > packets without direct xmit, as in PF_PACKET we don't have a notion of
> > > > "flow" but just raw packets instead, and can keep the mapping local
> > > > depending on the current CPU as we do queue setting elsewhere in the
> > > > stack just as well.
> > > 
> > > It seems to me that the newly added xmit in packet_sock is
> > > dev_queue_xmit() by default, and in this default case, dev_queue_xmit()
> > > would call netdev_pick_tx(), which would set the skb queue_mapping again
> > > to override the value based on the current CPU. 
> > 
> > Yes, I think you are right, that is also my experience with the code path.
> > 
> > > Or did I miss something here? 
> > 
> > A bit related; One thing I'm missing to understand, is why the
> > RAW/PF_PACKET sockets have a NULL in skb->sk when they reach
> > __netdev_pick_tx() ? (resulting in they cannot store/cache the queue in
> > sk_tx_queue_set)
> 
> I checked the code, it seems skb->sk is not set in this code path (for
> tcp, seems tcp_transmit_skb() sets it). 
> 
> Do you think we could set it here where skb_set_queue_mapping() is used,
> so for this code path, we could also have it cached(but for devices
> which define their queue selection method, it will not take effect)?

Sorry, seems it will not take effect, as the sk_tx_queue_set/get logic
depends on sk->sk_dst_cache, which is used by tcp, connected udp ...

Not sure why we need this dependency ... 

Thanks, Zhong

> something like below: 
> 
> @@ -2219,7 +2219,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  			}
>  		}
>  
> -		skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
> +		skb->sk = (struct sock *)po;
>  		skb->destructor = tpacket_destruct_skb;
>  		__packet_set_status(po, ph, TP_STATUS_SENDING);
>  		atomic_inc(&po->tx_ring.pending);
> @@ -2429,7 +2429,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  	skb->dev = dev;
>  	skb->priority = sk->sk_priority;
>  	skb->mark = sk->sk_mark;
> -	skb_set_queue_mapping(skb, packet_pick_tx_queue(dev));
> +	skb->sk = sk;
>  
>  	if (po->has_vnet_hdr) {
>  		if (vnet_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> 
> 
>