netdev - Re: [v2 RFC PATCH 2/4] Changes for virtio-net

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 17 Sep 2010 17:57:54 +0530
From:	Krishna Kumar2 <krkumar2@...ibm.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	anthony@...emonkey.ws, arnd@...db.de, avi@...hat.com,
	davem@...emloft.net, kvm@...r.kernel.org, mst@...hat.com,
	netdev@...r.kernel.org, rusty@...tcorp.com.au
Subject: Re: [v2 RFC PATCH 2/4] Changes for virtio-net

Eric Dumazet <eric.dumazet@...il.com> wrote on 09/17/2010 03:55:54 PM:

> > +/* Our representation of a send virtqueue */
> > +struct send_queue {
> > +   struct virtqueue *svq;
> > +
> > +   /* TX: fragments + linear part + virtio header */
> > +   struct scatterlist tx_sg[MAX_SKB_FRAGS + 2];
> > +};
>
> You probably want ____cacheline_aligned_in_smp

I had tried this and mentioned this in Patch 0/4:
"2. Cache-align data structures: I didn't see any BW/SD improvement
   after making the sq's (and similarly for vhost) cache-aligned
   statically:
        struct virtnet_info {
                ...
                struct send_queue sq[16] ____cacheline_aligned_in_smp;
                ...
        };
"

I am not sure why this made no difference?

> > +
> >  struct virtnet_info {
> >     struct virtio_device *vdev;
> > -   struct virtqueue *rvq, *svq, *cvq;
> > +   int numtxqs;         /* Number of tx queues */
> > +   struct send_queue *sq;
> > +   struct virtqueue *rvq;
> > +   struct virtqueue *cvq;
> >     struct net_device *dev;
>
> struct napi will probably be dirtied by RX processing
>
> You should make sure it doesnt dirty cache line of above (read mostly)
> fields

I am changing the layout of napi wrt other pointers in
this patch, though the to-be-submitted RX patch does that.
Should I do something for this TX-only patch?

> > +#define MAX_DEVICE_NAME      16
> > +static int initialize_vqs(struct virtnet_info *vi, int numtxqs)
> > +{
> > +   vq_callback_t **callbacks;
> > +   struct virtqueue **vqs;
> > +   int i, err = -ENOMEM;
> > +   int totalvqs;
> > +   char **names;
> > +
> > +   /* Allocate send queues */
>
> no check on numtxqs ? Hmm...
>
> Please then use kcalloc(numtxqs, sizeof(*vi->sq), GFP_KERNEL) so that
> some check is done for you ;)

Right! I need to re-introduce some limit. Rusty, should I simply
add a check for a constant (like 256) here?

Thanks for your review, Eric!

- KK

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html