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

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

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