Re: [PATCH v2 0/3] virtio_ring: Clean up code for virtio ring and pci
Series:
Reviewed-by: Xuan Zhuo
Thanks.
On Wed, 15 Mar 2023 20:54:55 +0200, Feng Liu via Virtualization
wrote:
> This patch series performs a clean up of the code in virtio_ring and
> virtio_pci, modifying it to conform with the Linux kernel coding style
> guidance [1]. The modifications ensure the code easy to read and
> understand. This small series does few short cleanups in the code.
>
> Patch-1 Allow non power of 2 sizes for packed virtqueues.
> Patch-2 Avoid using inline for small functions.
> Patch-3 Use const to annotate read-only pointer params.
>
> [1]
> https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease
>
> All of the patches have been verified based on the kernel code
> commit 81ff855485a3 ("Merge tag 'i2c-for-6.3-rc2' of
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux")
>
> Feng Liu (3):
> virtio_ring: Allow non power of 2 sizes for packed virtqueue
> virtio_ring: Avoid using inline for small functions
> virtio_ring: Use const to annotate read-only pointer params
>
> drivers/virtio/virtio_pci_modern.c | 5
> drivers/virtio/virtio_ring.c | 48 +++---
> include/linux/virtio.h | 14 -
> 3 files changed, 31 insertions(+), 36 deletions(-)
>
> --
> 2.34.1
>
> ___
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 3/4] virtio: fix up virtio_disable_cb
On Thu, 30 Mar 2023 10:04:03 -0400, "Michael S. Tsirkin"
wrote:
> On Thu, Mar 30, 2023 at 02:54:21PM +0800, Xuan Zhuo wrote:
> > On Thu, 30 Mar 2023 02:44:44 -0400, "Michael S. Tsirkin"
> > wrote:
> > > On Thu, Mar 30, 2023 at 02:07:37PM +0800, Xuan Zhuo wrote:
> > > > On Wed, 26 May 2021 04:24:40 -0400, "Michael S. Tsirkin"
> > > > wrote:
> > > > > virtio_disable_cb is currently a nop for split ring with event index.
> > > > > This is because it used to be always called from a callback when we
> > > > > know
> > > > > device won't trigger more events until we update the index. However,
> > > > > now that we run with interrupts enabled a lot we also poll without a
> > > > > callback so that is different: disabling callbacks will help reduce
> > > > > the
> > > > > number of spurious interrupts.
> > > > > Further, if using event index with a packed ring, and if being called
> > > > > from a callback, we actually do disable interrupts which is
> > > > > unnecessary.
> > > > >
> > > > > Fix both issues by tracking whenever we get a callback. If that is
> > > > > the case disabling interrupts with event index can be a nop.
> > > > > If not the case disable interrupts. Note: with a split ring
> > > > > there's no explicit "no interrupts" value. For now we write
> > > > > a fixed value so our chance of triggering an interupt
> > > > > is 1/ring size. It's probably better to write something
> > > > > related to the last used index there to reduce the chance
> > > > > even further. For now I'm keeping it simple.
> > > >
> > > >
> > > > Don't understand, is this patch necessary? For this patch set, we can
> > > > do without
> > > > this patch.
> > > >
> > > > So doest this patch optimize virtqueue_disable_cb() by reducing a
> > > > modification of
> > > > vring_used_event(&vq-> split.vring)?
> > > >
> > > > Or I miss something.
> > > >
> > > > Thanks.
> > >
> > > Before this patch virtqueue_disable_cb did nothing at all
> > > for the common case of event index enabled, so
> > > calling it from virtio net would not help matters.
> >
> > I agree with these codes:
> >
> > - if (!vq->event)
> > + if (vq->event)
> > + /* TODO: this is a hack. Figure out a cleaner value to
> > write. */
> > + vring_used_event(&vq->split.vring) = 0x0;
> > + else
> >
> >
> > I just don't understand event_triggered.
>
>
> The comment near it says it all:
> /* Hint for event idx: already triggered no need to disable. */
> the write into event idx is potentially expensive since it can
> invalidate cache for another processor (depending on the CPU).
Yes, I agree.
Thanks.
>
> > >
> > > But the patch is from 2021, isn't it a bit too late to argue?
> > > If you have a cleanup or an optimization in mind, please
> > > post a patch.
> >
> > Sorry, I just have some problems, I don't oppose it. At least it can reduce
> > the
> > modification of vring_used_event(&vq->split.vring). I think it is also
> > beneficial.
> >
> > Thanks very much.
> >
> >
> > >
> > > > >
> > > > > Signed-off-by: Michael S. Tsirkin
> > > > > ---
> > > > > drivers/virtio/virtio_ring.c | 26 +-
> > > > > 1 file changed, 25 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c
> > > > > b/drivers/virtio/virtio_ring.c
> > > > > index 71e16b53e9c1..88f0b16b11b8 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -113,6 +113,9 @@ struct vring_virtqueue {
> > > > > /* Last used index we've seen. */
> > > > > u16 last_used_idx;
> > > > >
> > > > > + /* Hint for event idx: already triggered no need to disable. */
> > > > > + bool event_triggered;
> > > > > +
> > > > > union {
> > > > > /* Available for split ring */
> > > > > struct {
> > > > > @@ -739,7 +742,10 @@ static void virtqueue_disable_cb_split(struct
> > > > > virtqueue *_vq)
> > > > >
> > > > > if (!(vq->split.avail_flags_shadow &
> > > > > VRING_AVAIL_F_NO_INTERRUPT)) {
> > > > > vq->split.avail_flags_shadow |=
> > > > > VRING_AVAIL_F_NO_INTERRUPT;
> > > > > - if (!vq->event)
> > > > > + if (vq->event)
> > > > > + /* TODO: this is a hack. Figure out a cleaner
> > > > > value to write. */
> > > > > + vring_used_event(&vq->split.vring) = 0x0;
> > > > > + else
> > > > > vq->split.vring.avail->flags =
> > > > > cpu_to_virtio16(_vq->vdev,
> > > > >
> > > > > vq->split.avail_flags_shadow);
> > > > > @@ -1605,6 +1611,7 @@ static struct virtqueue
> > > > > *vring_create_virtqueue_packed(
> > > > > vq->weak_barriers = weak_barriers;
> > > > > vq->broken = false;
> > > > > vq->last_used_idx = 0;
> > > > > + vq->event_triggered = false;
> > > > >
Re: [PATCH v2 3/3] virtio_ring: Use const to annotate read-only pointer params
On 2023-03-30 p.m.4:27, Michael S. Tsirkin wrote:
External email: Use caution opening links or attachments
On Thu, Mar 30, 2023 at 02:22:44PM -0400, Feng Liu wrote:
On 2023-03-16 p.m.11:17, Jason Wang wrote:
External email: Use caution opening links or attachments
On Thu, Mar 16, 2023 at 2:55 AM Feng Liu wrote:
Add const to make the read-only pointer parameters clear, similar to
many existing functions.
Use `container_of_const` to implement `to_vvq`, which ensures the
const-ness of read-only parameters and avoids accidental modification
of their members.
Signed-off-by: Feng Liu
Reviewed-by: Jiri Pirko
Acked-by: Jason Wang
Thanks
Hi Michael & Jason
Could you please help to take these reviewed/acked patches forward?
Thanks so much
this is not going in this linux, only the next.
Hi, Michael
Thanks for the update.
---
v0 -> v1
feedbacks from Michael S. Tsirkin
- use `container_of_const` to implement `to_vvq`
---
drivers/virtio/virtio_ring.c | 36 ++--
include/linux/virtio.h | 14 +++---
2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a26fab91c59f..4c3bb0ddeb9b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
* Helpers.
*/
-#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
+#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
-static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
+static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
unsigned int total_sg)
{
/*
@@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue
*vq,
* unconditionally on data path.
*/
-static bool vring_use_dma_api(struct virtio_device *vdev)
+static bool vring_use_dma_api(const struct virtio_device *vdev)
{
if (!virtio_has_dma_quirk(vdev))
return true;
@@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
return false;
}
-size_t virtio_max_dma_size(struct virtio_device *vdev)
+size_t virtio_max_dma_size(const struct virtio_device *vdev)
{
size_t max_segment_size = SIZE_MAX;
@@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32
num)
*/
static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
- struct vring_desc *desc)
+ const struct vring_desc *desc)
{
u16 flags;
@@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
}
static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
-struct vring_desc_extra *extra)
+const struct vring_desc_extra *extra)
{
u16 flags;
@@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct
vring_virtqueue *vq,
}
static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
- struct vring_packed_desc *desc)
+ const struct vring_packed_desc *desc)
{
u16 flags;
@@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
* Returns the size of the vring. This is mainly used for boasting to
* userspace. Unlike other operations, this need not be serialized.
*/
-unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
+unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
{
- struct vring_virtqueue *vq = to_vvq(_vq);
+ const struct vring_virtqueue *vq = to_vvq(_vq);
return vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
}
@@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
}
EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
-bool virtqueue_is_broken(struct virtqueue *_vq)
+bool virtqueue_is_broken(const struct virtqueue *_vq)
{
- struct vring_virtqueue *vq = to_vvq(_vq);
+ const struct vring_virtqueue *vq = to_vvq(_vq);
return READ_ONCE(vq->broken);
}
@@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device *dev)
}
EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
-dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *_vq)
{
- struct vring_virtqueue *vq = to_vvq(_vq);
+ const struct vring_virtqueue *vq = to_vvq(_vq);
BUG_ON(!vq->we_own_ring);
@@ -2881,9 +2881,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
}
EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
-dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
{
- s
Re: [RFC PATCH v3 2/4] vsock/vmci: convert VMCI error code to -ENOMEM on receive
> On Mar 30, 2023, at 1:18 PM, Arseniy Krasnov wrote:
>
> !! External Email
>
> On 30.03.2023 23:13, Arseniy Krasnov wrote:
>> This adds conversion of VMCI specific error code to general -ENOMEM. It
>> is needed, because af_vsock.c passes error value returned from transport
>> to the user, which does not expect to get VMCI_ERROR_* values.
>
> @Stefano, I have some doubts about this commit message, as it says "...
> af_vsock.c
> passes error value returned from transport to the user ...", but this
> behaviour is implemented only in the next patch. Is it ok, if both patches
> are in a single patchset?
>
> For patch 1 I think it is ok, as it fixes current implementation.
>
> Thanks, Arseniy
>
>>
>> Signed-off-by: Arseniy Krasnov
Code change looks good to me. Will let you figure out the commit message
with Stefano. Thanks!
Reviewed-by: Vishnu Dasa
>> ---
>> net/vmw_vsock/vmci_transport.c | 11 +--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>> index 95cc4d79ba29..b370070194fa 100644
>> --- a/net/vmw_vsock/vmci_transport.c
>> +++ b/net/vmw_vsock/vmci_transport.c
>> @@ -1831,10 +1831,17 @@ static ssize_t vmci_transport_stream_dequeue(
>> size_t len,
>> int flags)
>> {
>> + ssize_t err;
>> +
>> if (flags & MSG_PEEK)
>> - return vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
>> + err = vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
>> else
>> - return vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
>> + err = vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
>> +
>> + if (err < 0)
>> + err = -ENOMEM;
>> +
>> + return err;
>> }
>>
>> static ssize_t vmci_transport_stream_enqueue(
>
> !! External Email: This email originated from outside of the organization. Do
> not click links or open attachments unless you recognize the sender.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v3 1/4] vsock/vmci: convert VMCI error code to -ENOMEM on send
> On Mar 30, 2023, at 1:12 PM, Arseniy Krasnov wrote:
>
> !! External Email
>
> This adds conversion of VMCI specific error code to general -ENOMEM. It
> is needed, because af_vsock.c passes error value returned from transport
> to the user, which does not expect to get VMCI_ERROR_* values.
>
> Fixes: c43170b7e157 ("vsock: return errors other than -ENOMEM to socket")
> Signed-off-by: Arseniy Krasnov
Thanks, looks good to me.
Reviewed-by: Vishnu Dasa
> ---
> net/vmw_vsock/vmci_transport.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> index 36eb16a40745..95cc4d79ba29 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -1842,7 +1842,13 @@ static ssize_t vmci_transport_stream_enqueue(
>struct msghdr *msg,
>size_t len)
> {
> - return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> + ssize_t err;
> +
> + err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> + if (err < 0)
> + err = -ENOMEM;
> +
> + return err;
> }
>
> static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
> --
> 2.25.1
>
> !! External Email: This email originated from outside of the organization. Do
> not click links or open attachments unless you recognize the sender.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/3] virtio_ring: Use const to annotate read-only pointer params
On Thu, Mar 30, 2023 at 02:22:44PM -0400, Feng Liu wrote:
>
>
> On 2023-03-16 p.m.11:17, Jason Wang wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Thu, Mar 16, 2023 at 2:55 AM Feng Liu wrote:
> > >
> > > Add const to make the read-only pointer parameters clear, similar to
> > > many existing functions.
> > >
> > > Use `container_of_const` to implement `to_vvq`, which ensures the
> > > const-ness of read-only parameters and avoids accidental modification
> > > of their members.
> > >
> > > Signed-off-by: Feng Liu
> > > Reviewed-by: Jiri Pirko
> >
> > Acked-by: Jason Wang
> >
> > Thanks
> >
> Hi Michael & Jason
> Could you please help to take these reviewed/acked patches forward?
> Thanks so much
this is not going in this linux, only the next.
>
> > >
> > > ---
> > > v0 -> v1
> > > feedbacks from Michael S. Tsirkin
> > > - use `container_of_const` to implement `to_vvq`
> > > ---
> > > drivers/virtio/virtio_ring.c | 36 ++--
> > > include/linux/virtio.h | 14 +++---
> > > 2 files changed, 25 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index a26fab91c59f..4c3bb0ddeb9b 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
> > >* Helpers.
> > >*/
> > >
> > > -#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> > > +#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
> > >
> > > -static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> > > +static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
> > > unsigned int total_sg)
> > > {
> > > /*
> > > @@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct
> > > vring_virtqueue *vq,
> > >* unconditionally on data path.
> > >*/
> > >
> > > -static bool vring_use_dma_api(struct virtio_device *vdev)
> > > +static bool vring_use_dma_api(const struct virtio_device *vdev)
> > > {
> > > if (!virtio_has_dma_quirk(vdev))
> > > return true;
> > > @@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device
> > > *vdev)
> > > return false;
> > > }
> > >
> > > -size_t virtio_max_dma_size(struct virtio_device *vdev)
> > > +size_t virtio_max_dma_size(const struct virtio_device *vdev)
> > > {
> > > size_t max_segment_size = SIZE_MAX;
> > >
> > > @@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue
> > > *vq, u32 num)
> > >*/
> > >
> > > static void vring_unmap_one_split_indirect(const struct vring_virtqueue
> > > *vq,
> > > - struct vring_desc *desc)
> > > + const struct vring_desc *desc)
> > > {
> > > u16 flags;
> > >
> > > @@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
> > > }
> > >
> > > static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> > > -struct vring_desc_extra *extra)
> > > +const struct vring_desc_extra *extra)
> > > {
> > > u16 flags;
> > >
> > > @@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct
> > > vring_virtqueue *vq,
> > > }
> > >
> > > static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
> > > - struct vring_packed_desc *desc)
> > > + const struct vring_packed_desc *desc)
> > > {
> > > u16 flags;
> > >
> > > @@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
> > >* Returns the size of the vring. This is mainly used for boasting to
> > >* userspace. Unlike other operations, this need not be serialized.
> > >*/
> > > -unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
> > > +unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
> > > {
> > >
> > > - struct vring_virtqueue *vq = to_vvq(_vq);
> > > + const struct vring_virtqueue *vq = to_vvq(_vq);
> > >
> > > return vq->packed_ring ? vq->packed.vring.num :
> > > vq->split.vring.num;
> > > }
> > > @@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
> > > }
> > > EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
> > >
> > > -bool virtqueue_is_broken(struct virtqueue *_vq)
> > > +bool virtqueue_is_broken(const struct virtqueue *_vq)
> > > {
> > > - struct vring_virtqueue *vq = to_vvq(_vq);
> > > + const struct vring_virtqueue *vq = to_vvq(_vq);
> > >
> > > return READ_ONCE(vq->broken);
> > > }
> > > @@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device
> > > *dev)
> > > }
> > > EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
> > >
Re: [PATCH v2 3/3] virtio_ring: Use const to annotate read-only pointer params
On 2023-03-16 p.m.11:17, Jason Wang wrote:
External email: Use caution opening links or attachments
On Thu, Mar 16, 2023 at 2:55 AM Feng Liu wrote:
Add const to make the read-only pointer parameters clear, similar to
many existing functions.
Use `container_of_const` to implement `to_vvq`, which ensures the
const-ness of read-only parameters and avoids accidental modification
of their members.
Signed-off-by: Feng Liu
Reviewed-by: Jiri Pirko
Acked-by: Jason Wang
Thanks
Hi Michael & Jason
Could you please help to take these reviewed/acked patches forward?
Thanks so much
---
v0 -> v1
feedbacks from Michael S. Tsirkin
- use `container_of_const` to implement `to_vvq`
---
drivers/virtio/virtio_ring.c | 36 ++--
include/linux/virtio.h | 14 +++---
2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a26fab91c59f..4c3bb0ddeb9b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
* Helpers.
*/
-#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
+#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
-static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
+static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
unsigned int total_sg)
{
/*
@@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue
*vq,
* unconditionally on data path.
*/
-static bool vring_use_dma_api(struct virtio_device *vdev)
+static bool vring_use_dma_api(const struct virtio_device *vdev)
{
if (!virtio_has_dma_quirk(vdev))
return true;
@@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
return false;
}
-size_t virtio_max_dma_size(struct virtio_device *vdev)
+size_t virtio_max_dma_size(const struct virtio_device *vdev)
{
size_t max_segment_size = SIZE_MAX;
@@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32
num)
*/
static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
- struct vring_desc *desc)
+ const struct vring_desc *desc)
{
u16 flags;
@@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
}
static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
-struct vring_desc_extra *extra)
+const struct vring_desc_extra *extra)
{
u16 flags;
@@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct
vring_virtqueue *vq,
}
static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
- struct vring_packed_desc *desc)
+ const struct vring_packed_desc *desc)
{
u16 flags;
@@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
* Returns the size of the vring. This is mainly used for boasting to
* userspace. Unlike other operations, this need not be serialized.
*/
-unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
+unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
{
- struct vring_virtqueue *vq = to_vvq(_vq);
+ const struct vring_virtqueue *vq = to_vvq(_vq);
return vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
}
@@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
}
EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
-bool virtqueue_is_broken(struct virtqueue *_vq)
+bool virtqueue_is_broken(const struct virtqueue *_vq)
{
- struct vring_virtqueue *vq = to_vvq(_vq);
+ const struct vring_virtqueue *vq = to_vvq(_vq);
return READ_ONCE(vq->broken);
}
@@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device *dev)
}
EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
-dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *_vq)
{
- struct vring_virtqueue *vq = to_vvq(_vq);
+ const struct vring_virtqueue *vq = to_vvq(_vq);
BUG_ON(!vq->we_own_ring);
@@ -2881,9 +2881,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
}
EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
-dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
{
- struct vring_virtqueue *vq = to_vvq(_vq);
+ const struct vring_virtqueue *vq = to_vvq(_vq);
BUG_ON(!vq->we_own_ring);
@@ -2895,9 +2895,9 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
}
EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr);
-dma_addr_t virtqueue_get_used_addr(struct virtqu
Re: [PATCH v2 2/3] virtio_ring: Avoid using inline for small functions
On 2023-03-16 p.m.11:16, Jason Wang wrote:
External email: Use caution opening links or attachments
On Thu, Mar 16, 2023 at 2:55 AM Feng Liu wrote:
Remove the inline keyword, according to kernel coding style [1], defining
inline functions is not necessary for samll functions.
It is verified with GCC 12.2.0, the generated code with/without inline
is the same. Additionally tested with kernel pktgen and iperf, and
verified the result, pps of the results are the same in the cases of
with/without inline.
[1]
https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease
Signed-off-by: Feng Liu
Reviewed-by: Jiri Pirko
Acked-by: Jason Wang
Thanks
Hi Michael & Jason
Could you please help to take these reviewed/acked patches forward?
Thanks so much
---
drivers/virtio/virtio_ring.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 41144b5246a8..a26fab91c59f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -233,8 +233,8 @@ static void vring_free(struct virtqueue *_vq);
#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
-static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
- unsigned int total_sg)
+static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
+ unsigned int total_sg)
{
/*
* If the host supports indirect descriptor tables, and we have
multiple
@@ -349,7 +349,7 @@ static void vring_free_queue(struct virtio_device *vdev,
size_t size,
* making all of the arch DMA ops work on the vring device itself
* is a mess.
*/
-static inline struct device *vring_dma_dev(const struct vring_virtqueue *vq)
+static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
{
return vq->dma_dev;
}
@@ -784,7 +784,7 @@ static void detach_buf_split(struct vring_virtqueue *vq,
unsigned int head,
}
}
-static inline bool more_used_split(const struct vring_virtqueue *vq)
+static bool more_used_split(const struct vring_virtqueue *vq)
{
return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev,
vq->split.vring.used->idx);
@@ -1172,12 +1172,12 @@ static int virtqueue_resize_split(struct virtqueue
*_vq, u32 num)
/*
* Packed ring specific functions - *_packed().
*/
-static inline bool packed_used_wrap_counter(u16 last_used_idx)
+static bool packed_used_wrap_counter(u16 last_used_idx)
{
return !!(last_used_idx & (1 << VRING_PACKED_EVENT_F_WRAP_CTR));
}
-static inline u16 packed_last_used(u16 last_used_idx)
+static u16 packed_last_used(u16 last_used_idx)
{
return last_used_idx & ~(-(1 << VRING_PACKED_EVENT_F_WRAP_CTR));
}
@@ -1612,7 +1612,7 @@ static inline bool is_used_desc_packed(const struct
vring_virtqueue *vq,
return avail == used && used == used_wrap_counter;
}
-static inline bool more_used_packed(const struct vring_virtqueue *vq)
+static bool more_used_packed(const struct vring_virtqueue *vq)
{
u16 last_used;
u16 last_used_idx;
--
2.34.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/3] virtio_ring: Allow non power of 2 sizes for packed virtqueue
On 2023-03-16 p.m.11:16, Jason Wang wrote:
External email: Use caution opening links or attachments
On Thu, Mar 16, 2023 at 2:55 AM Feng Liu wrote:
According to the Virtio Specification, the Queue Size parameter of a
virtqueue corresponds to the maximum number of descriptors in that
queue, and it does not have to be a power of 2 for packed virtqueues.
However, the virtio_pci_modern driver enforced a power of 2 check for
virtqueue sizes, which is unnecessary and restrictive for packed
virtuqueue.
Split virtqueue still needs to check the virtqueue size is power_of_2
which has been done in vring_alloc_queue_split of the virtio_ring layer.
To validate this change, we tested various virtqueue sizes for packed
rings, including 128, 256, 512, 100, 200, 500, and 1000, with
CONFIG_PAGE_POISONING enabled, and all tests passed successfully.
Signed-off-by: Feng Liu
Reviewed-by: Jiri Pirko
Acked-by: Jason Wang
Thanks
Hi Michael & Jason
Could you please help to take these reviewed/acked patches forward?
Thanks so much
---
v0 -> v1
feedbacks from Jason Wang and Michael S. Tsirkin
- remove power_of_2 check of virtqueue size
v1 -> v2
feedbacks from Parav Pandit and Jiri Pirko
- keep power_of_2 check of split virtqueue in vring_alloc_queue_split of
virtio_ring layer.
---
drivers/virtio/virtio_pci_modern.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/virtio/virtio_pci_modern.c
b/drivers/virtio/virtio_pci_modern.c
index 9e496e288cfa..6e713904d8e8 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -310,11 +310,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device
*vp_dev,
if (!num || vp_modern_get_queue_enable(mdev, index))
return ERR_PTR(-ENOENT);
- if (!is_power_of_2(num)) {
- dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
- return ERR_PTR(-EINVAL);
- }
-
info->msix_vector = msix_vec;
/* create the vring */
--
2.34.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v2 2/3] vsock/vmci: convert VMCI error code to -ENOMEM
> On Mar 30, 2023, at 1:19 AM, Stefano Garzarella wrote:
>
> !! External Email
>
> On Thu, Mar 30, 2023 at 10:07:36AM +0300, Arseniy Krasnov wrote:
>> This adds conversion of VMCI specific error code to general -ENOMEM. It
>> is needed, because af_vsock.c passes error value returned from transport
>> to the user.
>>
>> Signed-off-by: Arseniy Krasnov
>> ---
>> net/vmw_vsock/vmci_transport.c | 19 ---
>> 1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
>> index 36eb16a40745..45de3e75597f 100644
>> --- a/net/vmw_vsock/vmci_transport.c
>> +++ b/net/vmw_vsock/vmci_transport.c
>> @@ -1831,10 +1831,17 @@ static ssize_t vmci_transport_stream_dequeue(
>> size_t len,
>> int flags)
>> {
>> + int err;
>
> Please, use the same type returned by the function.
>
>> +
>> if (flags & MSG_PEEK)
>> - return vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
>> + err = vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
>> else
>> - return vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
>> + err = vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
>> +
>> + if (err < 0)
>> + err = -ENOMEM;
>> +
>> + return err;
>> }
>>
>> static ssize_t vmci_transport_stream_enqueue(
>> @@ -1842,7 +1849,13 @@ static ssize_t vmci_transport_stream_enqueue(
>> struct msghdr *msg,
>> size_t len)
>> {
>> - return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>> + int err;
>
> Ditto.
>
>> +
>> + err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
>> + if (err < 0)
>> + err = -ENOMEM;
>> +
>> + return err;
>> }
>
> @Vishnu: should we backport the change for
> vmci_transport_stream_enqueue() to stable branches?
>
> In this case I would split this patch and I would send the
> vmci_transport_stream_enqueue() change to the net branch including:
>
> Fixes: c43170b7e157 ("vsock: return errors other than -ENOMEM to socket")
Yes, good point. It would be better to do it this way for correctness.
Thanks,
Vishnu
>
> Thanks,
> Stefano
>
>
> !! External Email: This email originated from outside of the organization. Do
> not click links or open attachments unless you recognize the sender.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 3/4] virtio: fix up virtio_disable_cb
On Thu, Mar 30, 2023 at 02:54:21PM +0800, Xuan Zhuo wrote:
> On Thu, 30 Mar 2023 02:44:44 -0400, "Michael S. Tsirkin"
> wrote:
> > On Thu, Mar 30, 2023 at 02:07:37PM +0800, Xuan Zhuo wrote:
> > > On Wed, 26 May 2021 04:24:40 -0400, "Michael S. Tsirkin"
> > > wrote:
> > > > virtio_disable_cb is currently a nop for split ring with event index.
> > > > This is because it used to be always called from a callback when we know
> > > > device won't trigger more events until we update the index. However,
> > > > now that we run with interrupts enabled a lot we also poll without a
> > > > callback so that is different: disabling callbacks will help reduce the
> > > > number of spurious interrupts.
> > > > Further, if using event index with a packed ring, and if being called
> > > > from a callback, we actually do disable interrupts which is unnecessary.
> > > >
> > > > Fix both issues by tracking whenever we get a callback. If that is
> > > > the case disabling interrupts with event index can be a nop.
> > > > If not the case disable interrupts. Note: with a split ring
> > > > there's no explicit "no interrupts" value. For now we write
> > > > a fixed value so our chance of triggering an interupt
> > > > is 1/ring size. It's probably better to write something
> > > > related to the last used index there to reduce the chance
> > > > even further. For now I'm keeping it simple.
> > >
> > >
> > > Don't understand, is this patch necessary? For this patch set, we can do
> > > without
> > > this patch.
> > >
> > > So doest this patch optimize virtqueue_disable_cb() by reducing a
> > > modification of
> > > vring_used_event(&vq-> split.vring)?
> > >
> > > Or I miss something.
> > >
> > > Thanks.
> >
> > Before this patch virtqueue_disable_cb did nothing at all
> > for the common case of event index enabled, so
> > calling it from virtio net would not help matters.
>
> I agree with these codes:
>
> - if (!vq->event)
> + if (vq->event)
> + /* TODO: this is a hack. Figure out a cleaner value to
> write. */
> + vring_used_event(&vq->split.vring) = 0x0;
> + else
>
>
> I just don't understand event_triggered.
The comment near it says it all:
/* Hint for event idx: already triggered no need to disable. */
the write into event idx is potentially expensive since it can
invalidate cache for another processor (depending on the CPU).
> >
> > But the patch is from 2021, isn't it a bit too late to argue?
> > If you have a cleanup or an optimization in mind, please
> > post a patch.
>
> Sorry, I just have some problems, I don't oppose it. At least it can reduce
> the
> modification of vring_used_event(&vq->split.vring). I think it is also
> beneficial.
>
> Thanks very much.
>
>
> >
> > > >
> > > > Signed-off-by: Michael S. Tsirkin
> > > > ---
> > > > drivers/virtio/virtio_ring.c | 26 +-
> > > > 1 file changed, 25 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 71e16b53e9c1..88f0b16b11b8 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -113,6 +113,9 @@ struct vring_virtqueue {
> > > > /* Last used index we've seen. */
> > > > u16 last_used_idx;
> > > >
> > > > + /* Hint for event idx: already triggered no need to disable. */
> > > > + bool event_triggered;
> > > > +
> > > > union {
> > > > /* Available for split ring */
> > > > struct {
> > > > @@ -739,7 +742,10 @@ static void virtqueue_disable_cb_split(struct
> > > > virtqueue *_vq)
> > > >
> > > > if (!(vq->split.avail_flags_shadow &
> > > > VRING_AVAIL_F_NO_INTERRUPT)) {
> > > > vq->split.avail_flags_shadow |=
> > > > VRING_AVAIL_F_NO_INTERRUPT;
> > > > - if (!vq->event)
> > > > + if (vq->event)
> > > > + /* TODO: this is a hack. Figure out a cleaner
> > > > value to write. */
> > > > + vring_used_event(&vq->split.vring) = 0x0;
> > > > + else
> > > > vq->split.vring.avail->flags =
> > > > cpu_to_virtio16(_vq->vdev,
> > > >
> > > > vq->split.avail_flags_shadow);
> > > > @@ -1605,6 +1611,7 @@ static struct virtqueue
> > > > *vring_create_virtqueue_packed(
> > > > vq->weak_barriers = weak_barriers;
> > > > vq->broken = false;
> > > > vq->last_used_idx = 0;
> > > > + vq->event_triggered = false;
> > > > vq->num_added = 0;
> > > > vq->packed_ring = true;
> > > > vq->use_dma_api = vring_use_dma_api(vdev);
> > > > @@ -1919,6 +1926,12 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
> > > > {
> > > > struct vring_virtqueue *vq = to_vvq(_vq);
> > > >
> > > > + /* If device t
Re: [PATCH net-next 8/8] virtio_net: introduce receive_small_xdp()
On Tue, 2023-03-28 at 20:04 +0800, Xuan Zhuo wrote:
> @@ -949,15 +1042,11 @@ static struct sk_buff *receive_small(struct net_device
> *dev,
> {
> struct sk_buff *skb;
> struct bpf_prog *xdp_prog;
> - unsigned int xdp_headroom = (unsigned long)ctx;
> - unsigned int header_offset = VIRTNET_RX_PAD + xdp_headroom;
> + unsigned int header_offset = VIRTNET_RX_PAD;
> unsigned int headroom = vi->hdr_len + header_offset;
This changes (reduces) the headroom for non-xpd-pass skbs.
[...]
> + buf += header_offset;
> + memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
AFAICS, that also means that receive_small(), for such packets, will
look for the virtio header in a different location. Is that expected?
Thanks.
Paolo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next 7/8] virtio_net: introduce receive_mergeable_xdp()
Hi,
On Tue, 2023-03-28 at 20:04 +0800, Xuan Zhuo wrote:
> The purpose of this patch is to simplify the receive_mergeable().
> Separate all the logic of XDP into a function.
>
> Signed-off-by: Xuan Zhuo
> ---
> drivers/net/virtio_net.c | 128 +++
> 1 file changed, 76 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 136131a7868a..c8978d8d8adb 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1316,6 +1316,63 @@ static void *mergeable_xdp_prepare(struct virtnet_info
> *vi,
> return page_address(xdp_page) + VIRTIO_XDP_HEADROOM;
> }
>
> +static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> + struct virtnet_info *vi,
> + struct receive_queue *rq,
> + struct bpf_prog *xdp_prog,
> + void *buf,
> + void *ctx,
> + unsigned int len,
> + unsigned int *xdp_xmit,
> + struct virtnet_rq_stats *stats)
> +{
> + struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
> + int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> + struct page *page = virt_to_head_page(buf);
> + int offset = buf - page_address(page);
> + unsigned int xdp_frags_truesz = 0;
> + struct sk_buff *head_skb;
> + unsigned int frame_sz;
> + struct xdp_buff xdp;
> + void *data;
> + u32 act;
> + int err;
> +
> + data = mergeable_xdp_prepare(vi, rq, xdp_prog, ctx, &frame_sz,
> &num_buf, &page,
> + offset, &len, hdr);
> + if (!data)
> + goto err_xdp;
> +
> + err = virtnet_build_xdp_buff_mrg(dev, vi, rq, &xdp, data, len, frame_sz,
> + &num_buf, &xdp_frags_truesz, stats);
> + if (unlikely(err))
> + goto err_xdp;
> +
> + act = virtnet_xdp_handler(xdp_prog, &xdp, dev, xdp_xmit, stats);
> +
> + switch (act) {
> + case VIRTNET_XDP_RES_PASS:
> + head_skb = build_skb_from_xdp_buff(dev, vi, &xdp,
> xdp_frags_truesz);
> + if (unlikely(!head_skb))
> + goto err_xdp;
> + return head_skb;
> +
> + case VIRTNET_XDP_RES_CONSUMED:
> + return NULL;
> +
> + case VIRTNET_XDP_RES_DROP:
> + break;
> + }
> +
> +err_xdp:
> + put_page(page);
> + mergeable_buf_free(rq, num_buf, dev, stats);
> +
> + stats->xdp_drops++;
> + stats->drops++;
> + return NULL;
> +}
> +
> static struct sk_buff *receive_mergeable(struct net_device *dev,
>struct virtnet_info *vi,
>struct receive_queue *rq,
> @@ -1325,21 +1382,22 @@ static struct sk_buff *receive_mergeable(struct
> net_device *dev,
>unsigned int *xdp_xmit,
>struct virtnet_rq_stats *stats)
> {
> - struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
> - int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> - struct page *page = virt_to_head_page(buf);
> - int offset = buf - page_address(page);
> - struct sk_buff *head_skb, *curr_skb;
> - struct bpf_prog *xdp_prog;
> unsigned int truesize = mergeable_ctx_to_truesize(ctx);
> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
> unsigned int tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
> unsigned int room = SKB_DATA_ALIGN(headroom + tailroom);
> - unsigned int frame_sz;
> - int err;
> + struct virtio_net_hdr_mrg_rxbuf *hdr;
> + struct sk_buff *head_skb, *curr_skb;
> + struct bpf_prog *xdp_prog;
> + struct page *page;
> + int num_buf;
> + int offset;
>
> head_skb = NULL;
> stats->bytes += len - vi->hdr_len;
> + hdr = buf;
> + num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> + page = virt_to_head_page(buf);
>
> if (unlikely(len > truesize - room)) {
> pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> @@ -1348,51 +1406,21 @@ static struct sk_buff *receive_mergeable(struct
> net_device *dev,
> goto err_skb;
> }
>
> - if (likely(!vi->xdp_enabled)) {
> - xdp_prog = NULL;
> - goto skip_xdp;
> - }
> -
> - rcu_read_lock();
> - xdp_prog = rcu_dereference(rq->xdp_prog);
> - if (xdp_prog) {
> - unsigned int xdp_frags_truesz = 0;
> - struct xdp_buff xdp;
> - void *data;
> - u32 act;
> -
> - data = mergeable_xdp_prepare(vi, rq, xdp_prog, ctx, &frame_sz,
> &num_buf, &page,
> - offset, &len
Re: [RFC PATCH v2 2/3] vsock/vmci: convert VMCI error code to -ENOMEM
On Thu, Mar 30, 2023 at 10:07:36AM +0300, Arseniy Krasnov wrote:
This adds conversion of VMCI specific error code to general -ENOMEM. It
is needed, because af_vsock.c passes error value returned from transport
to the user.
Signed-off-by: Arseniy Krasnov
---
net/vmw_vsock/vmci_transport.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
index 36eb16a40745..45de3e75597f 100644
--- a/net/vmw_vsock/vmci_transport.c
+++ b/net/vmw_vsock/vmci_transport.c
@@ -1831,10 +1831,17 @@ static ssize_t vmci_transport_stream_dequeue(
size_t len,
int flags)
{
+ int err;
Please, use the same type returned by the function.
+
if (flags & MSG_PEEK)
- return vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
+ err = vmci_qpair_peekv(vmci_trans(vsk)->qpair, msg, len, 0);
else
- return vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
+ err = vmci_qpair_dequev(vmci_trans(vsk)->qpair, msg, len, 0);
+
+ if (err < 0)
+ err = -ENOMEM;
+
+ return err;
}
static ssize_t vmci_transport_stream_enqueue(
@@ -1842,7 +1849,13 @@ static ssize_t vmci_transport_stream_enqueue(
struct msghdr *msg,
size_t len)
{
- return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
+ int err;
Ditto.
+
+ err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
+ if (err < 0)
+ err = -ENOMEM;
+
+ return err;
}
@Vishnu: should we backport the change for
vmci_transport_stream_enqueue() to stable branches?
In this case I would split this patch and I would send the
vmci_transport_stream_enqueue() change to the net branch including:
Fixes: c43170b7e157 ("vsock: return errors other than -ENOMEM to socket")
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH v2 1/3] vsock: return errors other than -ENOMEM to socket
On Thu, Mar 30, 2023 at 10:05:45AM +0300, Arseniy Krasnov wrote:
This removes behaviour, where error code returned from any transport
was always switched to ENOMEM. This works in the same way as:
commit
c43170b7e157 ("vsock: return errors other than -ENOMEM to socket"),
but for receive calls.
Signed-off-by: Arseniy Krasnov
---
net/vmw_vsock/af_vsock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
We should first make sure that all transports return the right value,
and then expose it to the user, so I would move this patch, after
patch 2.
Thanks,
Stefano
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 5f2dda35c980..413407bb646c 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -2043,7 +2043,7 @@ static int __vsock_stream_recvmsg(struct sock *sk, struct
msghdr *msg,
read = transport->stream_dequeue(vsk, msg, len - copied, flags);
if (read < 0) {
- err = -ENOMEM;
+ err = read;
break;
}
@@ -2094,7 +2094,7 @@ static int __vsock_seqpacket_recvmsg(struct sock *sk,
struct msghdr *msg,
msg_len = transport->seqpacket_dequeue(vsk, msg, flags);
if (msg_len < 0) {
- err = -ENOMEM;
+ err = msg_len;
goto out;
}
--
2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net v3] virtio/vsock: fix leaks due to missing skb owner
On Wed, Mar 29, 2023 at 04:51:58PM +, Bobby Eshleman wrote:
This patch sets the skb owner in the recv and send path for virtio.
For the send path, this solves the leak caused when
virtio_transport_purge_skbs() finds skb->sk is always NULL and therefore
never matches it with the current socket. Setting the owner upon
allocation fixes this.
For the recv path, this ensures correctness of accounting and also
correct transfer of ownership in vsock_loopback (when skbs are sent from
one socket and received by another).
Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
Signed-off-by: Bobby Eshleman
Reported-by: Cong Wang
Link: https://lore.kernel.org/all/ZCCbATwov4U+GBUv@pop-os.localdomain/
---
Changes in v3:
- virtio/vsock: use skb_set_owner_sk_safe() instead of
skb_set_owner_{r,w}
- virtio/vsock: reject allocating/receiving skb if sk_refcnt==0 and WARN_ONCE
- Link to v2:
https://lore.kernel.org/r/20230327-vsock-fix-leak-v2-1-f6619972d...@bytedance.com
Changes in v2:
- virtio/vsock: add skb_set_owner_r to recv_pkt()
- Link to v1:
https://lore.kernel.org/r/20230327-vsock-fix-leak-v1-1-3fede3671...@bytedance.com
---
net/vmw_vsock/virtio_transport_common.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/net/vmw_vsock/virtio_transport_common.c
b/net/vmw_vsock/virtio_transport_common.c
index 957cdc01c8e8..c927dc302faa 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -94,6 +94,11 @@ virtio_transport_alloc_skb(struct virtio_vsock_pkt_info
*info,
info->op,
info->flags);
+ if (info->vsk && !skb_set_owner_sk_safe(skb, sk_vsock(info->vsk))) {
+ WARN_ONCE(1, "failed to allocate skb on vsock socket with sk_refcnt
== 0\n");
+ goto out;
+ }
+
return skb;
out:
@@ -1294,6 +1299,11 @@ void virtio_transport_recv_pkt(struct virtio_transport
*t,
goto free_pkt;
}
+ if (!skb_set_owner_sk_safe(skb, sk)) {
+ WARN_ONCE(1, "receiving vsock socket has sk_refcnt == 0\n");
+ goto free_pkt;
+ }
+
LGTM!
I would have put the condition inside WARN_ONCE() because I find it
more readable (e.g. WARN_ONCE(!skb_set_owner_sk_safe(skb, sk), ...),
but I don't have a strong opinion on that, so that's fine too:
Reviewed-by: Stefano Garzarella
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
