From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B4929C6778D for ; Wed, 12 Sep 2018 16:12:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6AD3520880 for ; Wed, 12 Sep 2018 16:12:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6AD3520880 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728321AbeILVSG (ORCPT ); Wed, 12 Sep 2018 17:18:06 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:32956 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727004AbeILVSG (ORCPT ); Wed, 12 Sep 2018 17:18:06 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C0E47401EF07; Wed, 12 Sep 2018 16:12:51 +0000 (UTC) Received: from redhat.com (ovpn-121-244.rdu2.redhat.com [10.10.121.244]) by smtp.corp.redhat.com (Postfix) with SMTP id 42C3350332; Wed, 12 Sep 2018 16:12:49 +0000 (UTC) Date: Wed, 12 Sep 2018 12:12:48 -0400 From: "Michael S. Tsirkin" To: Tiwei Bie Cc: jasowang@redhat.com, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, virtio-dev@lists.oasis-open.org, wexu@redhat.com, jfreimann@redhat.com Subject: Re: [PATCH net-next v2 2/5] virtio_ring: support creating packed ring Message-ID: <20180912120900-mutt-send-email-mst@kernel.org> References: <20180711022711.7090-1-tiwei.bie@intel.com> <20180711022711.7090-3-tiwei.bie@intel.com> <20180907095208-mutt-send-email-mst@kernel.org> <20180910022837.GA11822@debian> <20180912075140.GA22545@debian> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180912075140.GA22545@debian> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Wed, 12 Sep 2018 16:12:51 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Wed, 12 Sep 2018 16:12:51 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'mst@redhat.com' RCPT:'' Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 12, 2018 at 03:51:40PM +0800, Tiwei Bie wrote: > On Mon, Sep 10, 2018 at 10:28:37AM +0800, Tiwei Bie wrote: > > On Fri, Sep 07, 2018 at 10:03:24AM -0400, Michael S. Tsirkin wrote: > > > On Wed, Jul 11, 2018 at 10:27:08AM +0800, Tiwei Bie wrote: > > > > This commit introduces the support for creating packed ring. > > > > All split ring specific functions are added _split suffix. > > > > Some necessary stubs for packed ring are also added. > > > > > > > > Signed-off-by: Tiwei Bie > > > > [...] > > > > + > > > > +/* > > > > + * The layout for the packed ring is a continuous chunk of memory > > > > + * which looks like this. > > > > + * > > > > + * struct vring_packed { > > > > + * // The actual descriptors (16 bytes each) > > > > + * struct vring_packed_desc desc[num]; > > > > + * > > > > + * // Padding to the next align boundary. > > > > + * char pad[]; > > > > + * > > > > + * // Driver Event Suppression > > > > + * struct vring_packed_desc_event driver; > > > > + * > > > > + * // Device Event Suppression > > > > + * struct vring_packed_desc_event device; > > > > + * }; > > > > + */ > > > > > > Why not just allocate event structures separately? > > > Is it a win to have them share a cache line for some reason? > > Users may call vring_new_virtqueue() with preallocated > memory to setup the ring, e.g.: > > https://github.com/torvalds/linux/blob/11da3a7f84f1/drivers/s390/virtio/virtio_ccw.c#L513-L522 > https://github.com/torvalds/linux/blob/11da3a7f84f1/drivers/misc/mic/vop/vop_main.c#L306-L307 > > Below is the corresponding definition in split ring: > > https://github.com/oasis-tcs/virtio-spec/blob/89dd55f5e606/split-ring.tex#L64-L78 > > If my understanding is correct, this is just for legacy > interfaces, and we won't define layout in packed ring > and don't need to support vring_new_virtqueue() in packed > ring. Is it correct? Thanks! mic doesn't support 1.0 yet but ccw does. It's probably best to look into converting ccw to virtio_create_virtqueue, then you can just fail vring_new_virtqueue for packed. Cornelia, are there any gotchas to look out for? > > > > > > Will do that. > > > > > > > > > +static inline void vring_init_packed(struct vring_packed *vr, unsigned int num, > > > > + void *p, unsigned long align) > > > > +{ > > > > + vr->num = num; > > > > + vr->desc = p; > > > > + vr->driver = (void *)ALIGN(((uintptr_t)p + > > > > + sizeof(struct vring_packed_desc) * num), align); > > > > + vr->device = vr->driver + 1; > > > > +} > > > > > > What's all this about alignment? Where does it come from? > > > > It comes from the `vring_align` parameter of vring_create_virtqueue() > > and vring_new_virtqueue(): > > > > https://github.com/torvalds/linux/blob/a49a9dcce802/drivers/virtio/virtio_ring.c#L1061 > > https://github.com/torvalds/linux/blob/a49a9dcce802/drivers/virtio/virtio_ring.c#L1123 > > > > Should I just ignore it in packed ring? > > > > CCW defined this: > > > > #define KVM_VIRTIO_CCW_RING_ALIGN 4096 > > > > I'm not familiar with CCW. Currently, in this patch set, packed ring > > isn't enabled on CCW dues to some legacy accessors are not implemented > > in packed ring yet. > > > > > > > > > + > > > > +static inline unsigned vring_size_packed(unsigned int num, unsigned long align) > > > > +{ > > > > + return ((sizeof(struct vring_packed_desc) * num + align - 1) > > > > + & ~(align - 1)) + sizeof(struct vring_packed_desc_event) * 2; > > > > +} > > [...] > > > > @@ -1129,10 +1388,17 @@ struct virtqueue *vring_new_virtqueue(unsigned int index, > > > > void (*callback)(struct virtqueue *vq), > > > > const char *name) > > > > { > > > > - struct vring vring; > > > > - vring_init(&vring, num, pages, vring_align); > > > > - return __vring_new_virtqueue(index, vring, vdev, weak_barriers, context, > > > > - notify, callback, name); > > > > + union vring_union vring; > > > > + bool packed; > > > > + > > > > + packed = virtio_has_feature(vdev, VIRTIO_F_RING_PACKED); > > > > + if (packed) > > > > + vring_init_packed(&vring.vring_packed, num, pages, vring_align); > > > > + else > > > > + vring_init(&vring.vring_split, num, pages, vring_align); > > > > > > > > > vring_init in the UAPI header is more or less a bug. > > > I'd just stop using it, keep it around for legacy userspace. > > > > Got it. I'd like to do that. Thanks. > > > > > > > > > + > > > > + return __vring_new_virtqueue(index, vring, packed, vdev, weak_barriers, > > > > + context, notify, callback, name); > > > > } > > > > EXPORT_SYMBOL_GPL(vring_new_virtqueue); > > > > > > > > @@ -1142,7 +1408,9 @@ void vring_del_virtqueue(struct virtqueue *_vq) > > > > > > > > if (vq->we_own_ring) { > > > > vring_free_queue(vq->vq.vdev, vq->queue_size_in_bytes, > > > > - vq->vring.desc, vq->queue_dma_addr); > > > > + vq->packed ? (void *)vq->vring_packed.desc : > > > > + (void *)vq->vring.desc, > > > > + vq->queue_dma_addr); > > > > } > > > > list_del(&_vq->list); > > > > kfree(vq); > > > > @@ -1184,7 +1452,7 @@ unsigned int virtqueue_get_vring_size(struct virtqueue *_vq) > > > > > > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > > > > > - return vq->vring.num; > > > > + return vq->packed ? vq->vring_packed.num : vq->vring.num; > > > > } > > > > EXPORT_SYMBOL_GPL(virtqueue_get_vring_size); > > > > > > > > @@ -1227,6 +1495,10 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq) > > > > > > > > BUG_ON(!vq->we_own_ring); > > > > > > > > + if (vq->packed) > > > > + return vq->queue_dma_addr + ((char *)vq->vring_packed.driver - > > > > + (char *)vq->vring_packed.desc); > > > > + > > > > return vq->queue_dma_addr + > > > > ((char *)vq->vring.avail - (char *)vq->vring.desc); > > > > } > > > > @@ -1238,11 +1510,16 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq) > > > > > > > > BUG_ON(!vq->we_own_ring); > > > > > > > > + if (vq->packed) > > > > + return vq->queue_dma_addr + ((char *)vq->vring_packed.device - > > > > + (char *)vq->vring_packed.desc); > > > > + > > > > return vq->queue_dma_addr + > > > > ((char *)vq->vring.used - (char *)vq->vring.desc); > > > > } > > > > EXPORT_SYMBOL_GPL(virtqueue_get_used_addr); > > > > > > > > +/* Only available for split ring */ > > > > const struct vring *virtqueue_get_vring(struct virtqueue *vq) > > > > { > > > > return &to_vvq(vq)->vring; > > > > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > > > > index fab02133a919..992142b35f55 100644 > > > > --- a/include/linux/virtio_ring.h > > > > +++ b/include/linux/virtio_ring.h > > > > @@ -60,6 +60,11 @@ static inline void virtio_store_mb(bool weak_barriers, > > > > struct virtio_device; > > > > struct virtqueue; > > > > > > > > +union vring_union { > > > > + struct vring vring_split; > > > > + struct vring_packed vring_packed; > > > > +}; > > > > + > > > > /* > > > > * Creates a virtqueue and allocates the descriptor ring. If > > > > * may_reduce_num is set, then this may allocate a smaller ring than > > > > @@ -79,7 +84,8 @@ struct virtqueue *vring_create_virtqueue(unsigned int index, > > > > > > > > /* Creates a virtqueue with a custom layout. */ > > > > struct virtqueue *__vring_new_virtqueue(unsigned int index, > > > > - struct vring vring, > > > > + union vring_union vring, > > > > + bool packed, > > > > struct virtio_device *vdev, > > > > bool weak_barriers, > > > > bool ctx, > > > > -- > > > > 2.18.0 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-dev-return-4842-cohuck=redhat.com@lists.oasis-open.org Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id E0C75985D24 for ; Wed, 12 Sep 2018 16:12:52 +0000 (UTC) Date: Wed, 12 Sep 2018 12:12:48 -0400 From: "Michael S. Tsirkin" Message-ID: <20180912120900-mutt-send-email-mst@kernel.org> References: <20180711022711.7090-1-tiwei.bie@intel.com> <20180711022711.7090-3-tiwei.bie@intel.com> <20180907095208-mutt-send-email-mst@kernel.org> <20180910022837.GA11822@debian> <20180912075140.GA22545@debian> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180912075140.GA22545@debian> Subject: [virtio-dev] Re: [PATCH net-next v2 2/5] virtio_ring: support creating packed ring To: Tiwei Bie Cc: jasowang@redhat.com, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, virtio-dev@lists.oasis-open.org, wexu@redhat.com, jfreimann@redhat.com List-ID: On Wed, Sep 12, 2018 at 03:51:40PM +0800, Tiwei Bie wrote: > On Mon, Sep 10, 2018 at 10:28:37AM +0800, Tiwei Bie wrote: > > On Fri, Sep 07, 2018 at 10:03:24AM -0400, Michael S. Tsirkin wrote: > > > On Wed, Jul 11, 2018 at 10:27:08AM +0800, Tiwei Bie wrote: > > > > This commit introduces the support for creating packed ring. > > > > All split ring specific functions are added _split suffix. > > > > Some necessary stubs for packed ring are also added. > > > > > > > > Signed-off-by: Tiwei Bie > > > > [...] > > > > + > > > > +/* > > > > + * The layout for the packed ring is a continuous chunk of memory > > > > + * which looks like this. > > > > + * > > > > + * struct vring_packed { > > > > + * // The actual descriptors (16 bytes each) > > > > + * struct vring_packed_desc desc[num]; > > > > + * > > > > + * // Padding to the next align boundary. > > > > + * char pad[]; > > > > + * > > > > + * // Driver Event Suppression > > > > + * struct vring_packed_desc_event driver; > > > > + * > > > > + * // Device Event Suppression > > > > + * struct vring_packed_desc_event device; > > > > + * }; > > > > + */ > > > > > > Why not just allocate event structures separately? > > > Is it a win to have them share a cache line for some reason? > > Users may call vring_new_virtqueue() with preallocated > memory to setup the ring, e.g.: > > https://github.com/torvalds/linux/blob/11da3a7f84f1/drivers/s390/virtio/virtio_ccw.c#L513-L522 > https://github.com/torvalds/linux/blob/11da3a7f84f1/drivers/misc/mic/vop/vop_main.c#L306-L307 > > Below is the corresponding definition in split ring: > > https://github.com/oasis-tcs/virtio-spec/blob/89dd55f5e606/split-ring.tex#L64-L78 > > If my understanding is correct, this is just for legacy > interfaces, and we won't define layout in packed ring > and don't need to support vring_new_virtqueue() in packed > ring. Is it correct? Thanks! mic doesn't support 1.0 yet but ccw does. It's probably best to look into converting ccw to virtio_create_virtqueue, then you can just fail vring_new_virtqueue for packed. Cornelia, are there any gotchas to look out for? > > > > > > Will do that. > > > > > > > > > +static inline void vring_init_packed(struct vring_packed *vr, unsigned int num, > > > > + void *p, unsigned long align) > > > > +{ > > > > + vr->num = num; > > > > + vr->desc = p; > > > > + vr->driver = (void *)ALIGN(((uintptr_t)p + > > > > + sizeof(struct vring_packed_desc) * num), align); > > > > + vr->device = vr->driver + 1; > > > > +} > > > > > > What's all this about alignment? Where does it come from? > > > > It comes from the `vring_align` parameter of vring_create_virtqueue() > > and vring_new_virtqueue(): > > > > https://github.com/torvalds/linux/blob/a49a9dcce802/drivers/virtio/virtio_ring.c#L1061 > > https://github.com/torvalds/linux/blob/a49a9dcce802/drivers/virtio/virtio_ring.c#L1123 > > > > Should I just ignore it in packed ring? > > > > CCW defined this: > > > > #define KVM_VIRTIO_CCW_RING_ALIGN 4096 > > > > I'm not familiar with CCW. Currently, in this patch set, packed ring > > isn't enabled on CCW dues to some legacy accessors are not implemented > > in packed ring yet. > > > > > > > > > + > > > > +static inline unsigned vring_size_packed(unsigned int num, unsigned long align) > > > > +{ > > > > + return ((sizeof(struct vring_packed_desc) * num + align - 1) > > > > + & ~(align - 1)) + sizeof(struct vring_packed_desc_event) * 2; > > > > +} > > [...] > > > > @@ -1129,10 +1388,17 @@ struct virtqueue *vring_new_virtqueue(unsigned int index, > > > > void (*callback)(struct virtqueue *vq), > > > > const char *name) > > > > { > > > > - struct vring vring; > > > > - vring_init(&vring, num, pages, vring_align); > > > > - return __vring_new_virtqueue(index, vring, vdev, weak_barriers, context, > > > > - notify, callback, name); > > > > + union vring_union vring; > > > > + bool packed; > > > > + > > > > + packed = virtio_has_feature(vdev, VIRTIO_F_RING_PACKED); > > > > + if (packed) > > > > + vring_init_packed(&vring.vring_packed, num, pages, vring_align); > > > > + else > > > > + vring_init(&vring.vring_split, num, pages, vring_align); > > > > > > > > > vring_init in the UAPI header is more or less a bug. > > > I'd just stop using it, keep it around for legacy userspace. > > > > Got it. I'd like to do that. Thanks. > > > > > > > > > + > > > > + return __vring_new_virtqueue(index, vring, packed, vdev, weak_barriers, > > > > + context, notify, callback, name); > > > > } > > > > EXPORT_SYMBOL_GPL(vring_new_virtqueue); > > > > > > > > @@ -1142,7 +1408,9 @@ void vring_del_virtqueue(struct virtqueue *_vq) > > > > > > > > if (vq->we_own_ring) { > > > > vring_free_queue(vq->vq.vdev, vq->queue_size_in_bytes, > > > > - vq->vring.desc, vq->queue_dma_addr); > > > > + vq->packed ? (void *)vq->vring_packed.desc : > > > > + (void *)vq->vring.desc, > > > > + vq->queue_dma_addr); > > > > } > > > > list_del(&_vq->list); > > > > kfree(vq); > > > > @@ -1184,7 +1452,7 @@ unsigned int virtqueue_get_vring_size(struct virtqueue *_vq) > > > > > > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > > > > > > - return vq->vring.num; > > > > + return vq->packed ? vq->vring_packed.num : vq->vring.num; > > > > } > > > > EXPORT_SYMBOL_GPL(virtqueue_get_vring_size); > > > > > > > > @@ -1227,6 +1495,10 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq) > > > > > > > > BUG_ON(!vq->we_own_ring); > > > > > > > > + if (vq->packed) > > > > + return vq->queue_dma_addr + ((char *)vq->vring_packed.driver - > > > > + (char *)vq->vring_packed.desc); > > > > + > > > > return vq->queue_dma_addr + > > > > ((char *)vq->vring.avail - (char *)vq->vring.desc); > > > > } > > > > @@ -1238,11 +1510,16 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq) > > > > > > > > BUG_ON(!vq->we_own_ring); > > > > > > > > + if (vq->packed) > > > > + return vq->queue_dma_addr + ((char *)vq->vring_packed.device - > > > > + (char *)vq->vring_packed.desc); > > > > + > > > > return vq->queue_dma_addr + > > > > ((char *)vq->vring.used - (char *)vq->vring.desc); > > > > } > > > > EXPORT_SYMBOL_GPL(virtqueue_get_used_addr); > > > > > > > > +/* Only available for split ring */ > > > > const struct vring *virtqueue_get_vring(struct virtqueue *vq) > > > > { > > > > return &to_vvq(vq)->vring; > > > > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h > > > > index fab02133a919..992142b35f55 100644 > > > > --- a/include/linux/virtio_ring.h > > > > +++ b/include/linux/virtio_ring.h > > > > @@ -60,6 +60,11 @@ static inline void virtio_store_mb(bool weak_barriers, > > > > struct virtio_device; > > > > struct virtqueue; > > > > > > > > +union vring_union { > > > > + struct vring vring_split; > > > > + struct vring_packed vring_packed; > > > > +}; > > > > + > > > > /* > > > > * Creates a virtqueue and allocates the descriptor ring. If > > > > * may_reduce_num is set, then this may allocate a smaller ring than > > > > @@ -79,7 +84,8 @@ struct virtqueue *vring_create_virtqueue(unsigned int index, > > > > > > > > /* Creates a virtqueue with a custom layout. */ > > > > struct virtqueue *__vring_new_virtqueue(unsigned int index, > > > > - struct vring vring, > > > > + union vring_union vring, > > > > + bool packed, > > > > struct virtio_device *vdev, > > > > bool weak_barriers, > > > > bool ctx, > > > > -- > > > > 2.18.0 --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org