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@xxxxxxxxxxxxxxxxxx> 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) {
--
To unsubscribe from this list: send the line "unsubscribe linux-next" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
