All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rusty Russell <rusty@rustcorp.com.au>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
	qemu-devel@nongnu.org, Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx
Date: Fri, 4 Jun 2010 12:04:57 +0930	[thread overview]
Message-ID: <201006041204.57973.rusty@rustcorp.com.au> (raw)
In-Reply-To: <10a74b58c908bad64ff890c881e2b2de88687f0e.1275403477.git.mst@redhat.com>

On Wed, 2 Jun 2010 12:17:12 am Michael S. Tsirkin wrote:
> This adds an (unused) option to put available ring before control (avail
> index, flags), and adds padding between index and flags. This avoids
> cache line sharing between control and ring, and also makes it possible
> to extend avail control without incurring extra cache misses.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

No no no no.  254?  You're trying to Morton me![1]

How's this (untested):

diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -74,8 +74,8 @@ struct vring {
 /* The standard layout for the ring is a continuous chunk of memory which looks
  * like this.  We assume num is a power of 2.
  *
- * struct vring
- * {
+ * struct vring {
+ *	*** The driver writes to this part.
  *	// The actual descriptors (16 bytes each)
  *	struct vring_desc desc[num];
  *
@@ -84,9 +84,11 @@ struct vring {
  *	__u16 avail_idx;
  *	__u16 available[num];
  *
- *	// Padding to the next align boundary.
+ *	// Padding so used_flags is on the next align boundary.
  *	char pad[];
+ *	__u16 last_used; // On a cacheline of its own.
  *
+ *	*** The device writes to this part.
  *	// A ring of used descriptor heads with free-running index.
  *	__u16 used_flags;
  *	__u16 used_idx;
@@ -110,6 +112,12 @@ static inline unsigned vring_size(unsign
 		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
 }
 
+/* Last used index sits at the very end of the driver part of the struct */
+static inline __u16 *vring_last_used_idx(const struct vring *vr)
+{
+	return (__u16 *)vr->used - 1;
+}
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Cheers,
Rusty.
[1] Andrew Morton has this technique where he posts a solution so ugly it
    forces others to fix it properly.  Ego-roping, basically.

WARNING: multiple messages have this Message-ID (diff)
From: Rusty Russell <rusty@rustcorp.com.au>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: qemu-devel@nongnu.org, Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	virtualization@lists.linux-foundation.org
Subject: [Qemu-devel] Re: [PATCHv3 1/2] virtio: support layout with avail ring before idx
Date: Fri, 4 Jun 2010 12:04:57 +0930	[thread overview]
Message-ID: <201006041204.57973.rusty@rustcorp.com.au> (raw)
In-Reply-To: <10a74b58c908bad64ff890c881e2b2de88687f0e.1275403477.git.mst@redhat.com>

On Wed, 2 Jun 2010 12:17:12 am Michael S. Tsirkin wrote:
> This adds an (unused) option to put available ring before control (avail
> index, flags), and adds padding between index and flags. This avoids
> cache line sharing between control and ring, and also makes it possible
> to extend avail control without incurring extra cache misses.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

No no no no.  254?  You're trying to Morton me![1]

How's this (untested):

diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -74,8 +74,8 @@ struct vring {
 /* The standard layout for the ring is a continuous chunk of memory which looks
  * like this.  We assume num is a power of 2.
  *
- * struct vring
- * {
+ * struct vring {
+ *	*** The driver writes to this part.
  *	// The actual descriptors (16 bytes each)
  *	struct vring_desc desc[num];
  *
@@ -84,9 +84,11 @@ struct vring {
  *	__u16 avail_idx;
  *	__u16 available[num];
  *
- *	// Padding to the next align boundary.
+ *	// Padding so used_flags is on the next align boundary.
  *	char pad[];
+ *	__u16 last_used; // On a cacheline of its own.
  *
+ *	*** The device writes to this part.
  *	// A ring of used descriptor heads with free-running index.
  *	__u16 used_flags;
  *	__u16 used_idx;
@@ -110,6 +112,12 @@ static inline unsigned vring_size(unsign
 		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
 }
 
+/* Last used index sits at the very end of the driver part of the struct */
+static inline __u16 *vring_last_used_idx(const struct vring *vr)
+{
+	return (__u16 *)vr->used - 1;
+}
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Cheers,
Rusty.
[1] Andrew Morton has this technique where he posts a solution so ugly it
    forces others to fix it properly.  Ego-roping, basically.

  reply	other threads:[~2010-06-04  2:35 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-06-01 14:47 [PATCHv3 0/2] virtio: put last seen used index into ring itself Michael S. Tsirkin
2010-06-01 14:47 ` [Qemu-devel] " Michael S. Tsirkin
2010-06-01 14:47 ` [PATCHv3 1/2] virtio: support layout with avail ring before idx Michael S. Tsirkin
2010-06-01 14:47 ` Michael S. Tsirkin
2010-06-01 14:47   ` [Qemu-devel] " Michael S. Tsirkin
2010-06-04  2:34   ` Rusty Russell [this message]
2010-06-04  2:34     ` [Qemu-devel] " Rusty Russell
2010-06-04 10:35     ` Michael S. Tsirkin
2010-06-04 10:35     ` Michael S. Tsirkin
2010-06-04 10:35       ` [Qemu-devel] " Michael S. Tsirkin
2010-06-04 11:16       ` Rusty Russell
2010-06-04 11:16         ` [Qemu-devel] " Rusty Russell
2010-06-04 11:42         ` Michael S. Tsirkin
2010-06-04 11:42           ` [Qemu-devel] " Michael S. Tsirkin
2010-06-05  4:10           ` Rusty Russell
2010-06-05  4:10           ` Rusty Russell
2010-06-05  4:10             ` [Qemu-devel] " Rusty Russell
2010-06-06  9:11             ` Michael S. Tsirkin
2010-06-06  9:11             ` Michael S. Tsirkin
2010-06-06  9:11               ` [Qemu-devel] " Michael S. Tsirkin
2010-06-04 11:42         ` Michael S. Tsirkin
2010-06-04 11:16       ` Rusty Russell
2010-06-04  2:34   ` Rusty Russell
2010-06-01 14:47 ` [PATCHv3 2/2] virtio: publish used idx Michael S. Tsirkin
2010-06-01 14:47   ` [Qemu-devel] " Michael S. Tsirkin
2010-06-01 15:12 ` [PATCHv3 0/2] virtio: put last seen used index into ring itself Michael S. Tsirkin
2010-06-01 15:12 ` Michael S. Tsirkin
2010-06-01 15:12   ` [Qemu-devel] " Michael S. Tsirkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201006041204.57973.rusty@rustcorp.com.au \
    --to=rusty@rustcorp.com.au \
    --cc=akpm@linux-foundation.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.