Eric Dumazet <eric.dumazet@xxxxxxxxx> 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 kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
