Virtualization - Dec 2018 - [PATCH v2 2/5] VSOCK: support fill data to mergeable rx buffer in host

On 2018/12/13 3:09, David Miller wrote:
> From: jiangyiwen <jiangyiwen at huawei.com>
> Date: Wed, 12 Dec 2018 17:29:31 +0800
> 
>> diff --git a/include/uapi/linux/virtio_vsock.h
b/include/uapi/linux/virtio_vsock.h
>> index 1d57ed3..2292f30 100644
>> --- a/include/uapi/linux/virtio_vsock.h
>> +++ b/include/uapi/linux/virtio_vsock.h
>> @@ -63,6 +63,11 @@ struct virtio_vsock_hdr {
>>  	__le32	fwd_cnt;
>>  } __attribute__((packed));
>>
>> +/* It add mergeable rx buffers feature */
>> +struct virtio_vsock_mrg_rxbuf_hdr {
>> +	__le16  num_buffers;    /* number of mergeable rx buffers */
>> +} __attribute__((packed));
>> +
> 
> I know the rest of this file uses 'packed' but this attribute
should
> only be used if absolutely necessary as it incurs a
> non-trivial performance penalty for some architectures.
> 
> .
> 
Hi David,

I hope Host can fill fewer bytes into rx virtqueue, so
I keep structure virtio_vsock_mrg_rxbuf_hdr one byte
alignment.

Thanks,
Yiwen.

From: jiangyiwen <jiangyiwen at huawei.com>
Date: Thu, 13 Dec 2018 11:11:48 +0800
> I hope Host can fill fewer bytes into rx virtqueue, so
> I keep structure virtio_vsock_mrg_rxbuf_hdr one byte
> alignment.
The question is if this actully matters.

Do you know?

If the obejct this is embeeded inside of is at least 2 byte aligned,
you are marking it packed for nothing.

There are only %100 downsides to using the packed attribute.

Simply define your datastructures properly, with fixed sized types,
and all padding defined explicitly.

On 2018/12/13 13:59, David Miller wrote:
> From: jiangyiwen <jiangyiwen at huawei.com>
> Date: Thu, 13 Dec 2018 11:11:48 +0800
> 
>> I hope Host can fill fewer bytes into rx virtqueue, so
>> I keep structure virtio_vsock_mrg_rxbuf_hdr one byte
>> alignment.
> 
> The question is if this actully matters.
> 
> Do you know?
> 
> If the obejct this is embeeded inside of is at least 2 byte aligned,
> you are marking it packed for nothing.
> 
> There are only %100 downsides to using the packed attribute.
> 
> Simply define your datastructures properly, with fixed sized types,
> and all padding defined explicitly.
> 
> .
> 
Hi David,

Thanks a lot, I need to send number buffers from Host to Guest, so I think
we need to keep the structure size the same between host and guest.
But after your reminder, I feel my code may exist a serious problem,
that in mergeable mode, I send the total structure virtio_vsock_pkt
from Host to Guest, however, this structure size may be different
under different compilers (Guest and Host are different). Then, Guest
may parse the wrong packet length.

David, I want to ask if there is such a problem?

In addition, why I send total virtio_vsock_pkt structure from Host to Guest?
- In order to avoid to allocate virtio_vsock_pkt memory when receiving
  packets, in case of insufficient memory, it may have some advantages, and
  we may keep consistent with old version.

Thanks again,
Yiwen.

On Thu, Dec 13, 2018 at 11:11:48AM +0800, jiangyiwen
wrote:
> On 2018/12/13 3:09, David Miller wrote:
> > From: jiangyiwen <jiangyiwen at huawei.com>
> > Date: Wed, 12 Dec 2018 17:29:31 +0800
> > 
> >> diff --git a/include/uapi/linux/virtio_vsock.h
b/include/uapi/linux/virtio_vsock.h
> >> index 1d57ed3..2292f30 100644
> >> --- a/include/uapi/linux/virtio_vsock.h
> >> +++ b/include/uapi/linux/virtio_vsock.h
> >> @@ -63,6 +63,11 @@ struct virtio_vsock_hdr {
> >>  	__le32	fwd_cnt;
> >>  } __attribute__((packed));
> >>
> >> +/* It add mergeable rx buffers feature */
> >> +struct virtio_vsock_mrg_rxbuf_hdr {
> >> +	__le16  num_buffers;    /* number of mergeable rx buffers */
> >> +} __attribute__((packed));
> >> +
> > 
> > I know the rest of this file uses 'packed' but this attribute
should
> > only be used if absolutely necessary as it incurs a
> > non-trivial performance penalty for some architectures.
> > 
> > .
> > 
> 
> Hi David,
> 
> I hope Host can fill fewer bytes into rx virtqueue, so
> I keep structure virtio_vsock_mrg_rxbuf_hdr one byte
> alignment.
> 
> Thanks,
> Yiwen.
It doesn't work like this now though, does it?
Buffers are preallocated and they are always aligned.
So I do not see the point.

-- 
MST

On 2018/12/13 22:50, Michael S. Tsirkin wrote:
> On Thu, Dec 13, 2018 at 11:11:48AM +0800, jiangyiwen wrote:
>> On 2018/12/13 3:09, David Miller wrote:
>>> From: jiangyiwen <jiangyiwen at huawei.com>
>>> Date: Wed, 12 Dec 2018 17:29:31 +0800
>>>
>>>> diff --git a/include/uapi/linux/virtio_vsock.h
b/include/uapi/linux/virtio_vsock.h
>>>> index 1d57ed3..2292f30 100644
>>>> --- a/include/uapi/linux/virtio_vsock.h
>>>> +++ b/include/uapi/linux/virtio_vsock.h
>>>> @@ -63,6 +63,11 @@ struct virtio_vsock_hdr {
>>>>  	__le32	fwd_cnt;
>>>>  } __attribute__((packed));
>>>>
>>>> +/* It add mergeable rx buffers feature */
>>>> +struct virtio_vsock_mrg_rxbuf_hdr {
>>>> +	__le16  num_buffers;    /* number of mergeable rx buffers */
>>>> +} __attribute__((packed));
>>>> +
>>>
>>> I know the rest of this file uses 'packed' but this
attribute should
>>> only be used if absolutely necessary as it incurs a
>>> non-trivial performance penalty for some architectures.
>>>
>>> .
>>>
>>
>> Hi David,
>>
>> I hope Host can fill fewer bytes into rx virtqueue, so
>> I keep structure virtio_vsock_mrg_rxbuf_hdr one byte
>> alignment.
>>
>> Thanks,
>> Yiwen.
> 
> It doesn't work like this now though, does it?
> Buffers are preallocated and they are always aligned.
> So I do not see the point.
> 
Hi Michael,

Now my patch has a serious problem, I use virtio_vsock_pkt as
the transport header from host to guest, it will cause
guest parse the wrong packet length. Because this structure
size may be different under different compilers
(guest and host are different). I will solve the problem
in later version.

Thanks,
Yiwen.