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

[Li Zhong <zhong@linux.vnet.ibm.com>]

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) {