This callback controls setting of POLLIN,POLLRDNORM output bits
of poll() syscall,but in some cases,it is incorrectly to set it,
when socket has at least 1 bytes of available data. Use 'target'
which is already exists and equal to sk_rcvlowat in this case.
Signed-off-by: Arseniy Krasnov <[email protected]>
---
net/vmw_vsock/virtio_transport_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index ec2c2afbf0d0..591908740992 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -634,7 +634,7 @@ virtio_transport_notify_poll_in(struct vsock_sock *vsk,
size_t target,
bool *data_ready_now)
{
- if (vsock_stream_has_data(vsk))
+ if (vsock_stream_has_data(vsk) >= target)
*data_ready_now = true;
else
*data_ready_now = false;
--
2.25.1
On Mon, Jul 18, 2022 at 08:17:31AM +0000, Arseniy Krasnov wrote:
>This callback controls setting of POLLIN,POLLRDNORM output bits
>of poll() syscall,but in some cases,it is incorrectly to set it,
>when socket has at least 1 bytes of available data. Use 'target'
>which is already exists and equal to sk_rcvlowat in this case.
>
>Signed-off-by: Arseniy Krasnov <[email protected]>
>---
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index ec2c2afbf0d0..591908740992 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -634,7 +634,7 @@ virtio_transport_notify_poll_in(struct vsock_sock *vsk,
> size_t target,
> bool *data_ready_now)
> {
>- if (vsock_stream_has_data(vsk))
>+ if (vsock_stream_has_data(vsk) >= target)
> *data_ready_now = true;
> else
> *data_ready_now = false;
Perhaps we can take the opportunity to clean up the code in this way:
*data_ready_now = vsock_stream_has_data(vsk) >= target;
Anyway, I think we also need to fix the other transports (vmci and
hyperv), what do you think?
Thanks,
Stefano
On 19.07.2022 15:48, Stefano Garzarella wrote:
> On Mon, Jul 18, 2022 at 08:17:31AM +0000, Arseniy Krasnov wrote:
>> This callback controls setting of POLLIN,POLLRDNORM output bits
>> of poll() syscall,but in some cases,it is incorrectly to set it,
>> when socket has at least 1 bytes of available data. Use 'target'
>> which is already exists and equal to sk_rcvlowat in this case.
>>
>> Signed-off-by: Arseniy Krasnov <[email protected]>
>> ---
>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>> index ec2c2afbf0d0..591908740992 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -634,7 +634,7 @@ virtio_transport_notify_poll_in(struct vsock_sock *vsk,
>> size_t target,
>> bool *data_ready_now)
>> {
>> - if (vsock_stream_has_data(vsk))
>> + if (vsock_stream_has_data(vsk) >= target)
>> *data_ready_now = true;
>> else
>> *data_ready_now = false;
>
> Perhaps we can take the opportunity to clean up the code in this way:
>
> *data_ready_now = vsock_stream_has_data(vsk) >= target;
Ack
>
> Anyway, I think we also need to fix the other transports (vmci and hyperv), what do you think?
For vmci it is look clear to fix it. For hyperv i need to check it more, because it already
uses some internal target value.
>
> Thanks,
> Stefano
>
On Wed, Jul 20, 2022 at 05:38:03AM +0000, Arseniy Krasnov wrote:
>On 19.07.2022 15:48, Stefano Garzarella wrote:
>> On Mon, Jul 18, 2022 at 08:17:31AM +0000, Arseniy Krasnov wrote:
>>> This callback controls setting of POLLIN,POLLRDNORM output bits
>>> of poll() syscall,but in some cases,it is incorrectly to set it,
>>> when socket has at least 1 bytes of available data. Use 'target'
>>> which is already exists and equal to sk_rcvlowat in this case.
>>>
>>> Signed-off-by: Arseniy Krasnov <[email protected]>
>>> ---
>>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>>> index ec2c2afbf0d0..591908740992 100644
>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>> @@ -634,7 +634,7 @@ virtio_transport_notify_poll_in(struct vsock_sock *vsk,
>>> ??????????????? size_t target,
>>> ??????????????? bool *data_ready_now)
>>> {
>>> -??? if (vsock_stream_has_data(vsk))
>>> +??? if (vsock_stream_has_data(vsk) >= target)
>>> ??????? *data_ready_now = true;
>>> ????else
>>> ??????? *data_ready_now = false;
>>
>> Perhaps we can take the opportunity to clean up the code in this way:
>>
>> ????*data_ready_now = vsock_stream_has_data(vsk) >= target;
>Ack
>>
>> Anyway, I think we also need to fix the other transports (vmci and hyperv), what do you think?
>For vmci it is look clear to fix it. For hyperv i need to check it more, because it already
>uses some internal target value.
Yep, I see. Maybe you can pass `target` to hvs_channel_readable() and
use it as parameter of HVS_PKT_LEN().
@Dexuan what do you think?
Thanks,
Stefano
> From: Stefano Garzarella <[email protected]>
> Sent: Wednesday, July 20, 2022 1:23 AM
> ...
> On Wed, Jul 20, 2022 at 05:38:03AM +0000, Arseniy Krasnov wrote:
> >On 19.07.2022 15:48, Stefano Garzarella wrote:
> >> On Mon, Jul 18, 2022 at 08:17:31AM +0000, Arseniy Krasnov wrote:
> >>> This callback controls setting of POLLIN,POLLRDNORM output bits
> >>> of poll() syscall,but in some cases,it is incorrectly to set it,
> >>> when socket has at least 1 bytes of available data. Use 'target'
> >>> which is already exists and equal to sk_rcvlowat in this case.
> >>>
> >>> Signed-off-by: Arseniy Krasnov <[email protected]>
> >>> ---
> >>> net/vmw_vsock/virtio_transport_common.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> >>> index ec2c2afbf0d0..591908740992 100644
> >>> --- a/net/vmw_vsock/virtio_transport_common.c
> >>> +++ b/net/vmw_vsock/virtio_transport_common.c
> >>> @@ -634,7 +634,7 @@ virtio_transport_notify_poll_in(struct vsock_sock
> *vsk,
> >>> ??????????????? size_t target,
> >>> ??????????????? bool *data_ready_now)
> >>> {
> >>> -??? if (vsock_stream_has_data(vsk))
> >>> +??? if (vsock_stream_has_data(vsk) >= target)
> >>> ??????? *data_ready_now = true;
> >>> ????else
> >>> ??????? *data_ready_now = false;
> >>
> >> Perhaps we can take the opportunity to clean up the code in this way:
> >>
> >> ????*data_ready_now = vsock_stream_has_data(vsk) >= target;
> >Ack
> >>
> >> Anyway, I think we also need to fix the other transports (vmci and hyperv),
> >> what do you think?
> >For vmci it is look clear to fix it. For hyperv i need to check it more, because it
> > already uses some internal target value.
>
> Yep, I see. Maybe you can pass `target` to hvs_channel_readable() and
> use it as parameter of HVS_PKT_LEN().
>
> @Dexuan what do you think?
>
> Thanks,
> Stefano
Can we return "not supported" to set_rcvlowat for Hyper-V vsock? :-)
For Hyper-V vsock, it's easy to tell if there is at least 1 byte to read:
please refer to hvs_channel_readable(), but it's difficult to figure out
exactly how many bytes can be read.
In hvs_channel_readable(), hv_get_bytes_to_read() returns the total
bytes of 0, 1 or multiple Hyper-V vsock packets: each packet has a
24-byte header (see HVS_HEADER_LEN), the payload, some padding
bytes (if the payload length is not a multiple of 8), and 8 trailing
useless bytes.
It's hard to get the total payload length because there is no API in
include/linux/hyperv.h, drivers/hv/channel.c and
drivers/hv/ring_buffer.c that allows us to peek at the data in the
VMBus channel's ringbuffer.
We could add such a "peek" API in drivers/hv/channel.c (see the
non-peek version of the APIs in hvs_stream_dequeue():
hv_pkt_iter_first() and hv_pkt_iter_next()), and examine the whole
ringbuffe to figure out the exact total payload length, but I feel it may
not be worth the non-trivial complexity just to be POSIX-compliant --
nobody ever complained about this for the past 5 years :-) So I'm
wondering if we should allow Hyper-V vsock to not support the
set_rcvlowat op?
Thanks,
-- Dexuan
On 20.07.2022 21:54, Dexuan Cui wrote:
>> From: Stefano Garzarella <[email protected]>
>> Sent: Wednesday, July 20, 2022 1:23 AM
>> ...
>> On Wed, Jul 20, 2022 at 05:38:03AM +0000, Arseniy Krasnov wrote:
>>> On 19.07.2022 15:48, Stefano Garzarella wrote:
>>>> On Mon, Jul 18, 2022 at 08:17:31AM +0000, Arseniy Krasnov wrote:
>>>>> This callback controls setting of POLLIN,POLLRDNORM output bits
>>>>> of poll() syscall,but in some cases,it is incorrectly to set it,
>>>>> when socket has at least 1 bytes of available data. Use 'target'
>>>>> which is already exists and equal to sk_rcvlowat in this case.
>>>>>
>>>>> Signed-off-by: Arseniy Krasnov <[email protected]>
>>>>> ---
>>>>> net/vmw_vsock/virtio_transport_common.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/vmw_vsock/virtio_transport_common.c
>> b/net/vmw_vsock/virtio_transport_common.c
>>>>> index ec2c2afbf0d0..591908740992 100644
>>>>> --- a/net/vmw_vsock/virtio_transport_common.c
>>>>> +++ b/net/vmw_vsock/virtio_transport_common.c
>>>>> @@ -634,7 +634,7 @@ virtio_transport_notify_poll_in(struct vsock_sock
>> *vsk,
>>>>> size_t target,
>>>>> bool *data_ready_now)
>>>>> {
>>>>> - if (vsock_stream_has_data(vsk))
>>>>> + if (vsock_stream_has_data(vsk) >= target)
>>>>> *data_ready_now = true;
>>>>> else
>>>>> *data_ready_now = false;
>>>>
>>>> Perhaps we can take the opportunity to clean up the code in this way:
>>>>
>>>> *data_ready_now = vsock_stream_has_data(vsk) >= target;
>>> Ack
>>>>
>>>> Anyway, I think we also need to fix the other transports (vmci and hyperv),
>>>> what do you think?
>>> For vmci it is look clear to fix it. For hyperv i need to check it more, because it
>>> already uses some internal target value.
>>
>> Yep, I see. Maybe you can pass `target` to hvs_channel_readable() and
>> use it as parameter of HVS_PKT_LEN().
>>
>> @Dexuan what do you think?
>>
>> Thanks,
>> Stefano
>
> Can we return "not supported" to set_rcvlowat for Hyper-V vsock? :-)
>
> For Hyper-V vsock, it's easy to tell if there is at least 1 byte to read:
> please refer to hvs_channel_readable(), but it's difficult to figure out
> exactly how many bytes can be read.
>
> In hvs_channel_readable(), hv_get_bytes_to_read() returns the total
> bytes of 0, 1 or multiple Hyper-V vsock packets: each packet has a
> 24-byte header (see HVS_HEADER_LEN), the payload, some padding
> bytes (if the payload length is not a multiple of 8), and 8 trailing
> useless bytes.
>
> It's hard to get the total payload length because there is no API in
> include/linux/hyperv.h, drivers/hv/channel.c and
> drivers/hv/ring_buffer.c that allows us to peek at the data in the
> VMBus channel's ringbuffer.
>
> We could add such a "peek" API in drivers/hv/channel.c (see the
> non-peek version of the APIs in hvs_stream_dequeue():
> hv_pkt_iter_first() and hv_pkt_iter_next()), and examine the whole
> ringbuffe to figure out the exact total payload length, but I feel it may
> not be worth the non-trivial complexity just to be POSIX-compliant --
> nobody ever complained about this for the past 5 years :-) So I'm
> wondering if we should allow Hyper-V vsock to not support the
> set_rcvlowat op?
I see, i can prepare patch for this in this patchset in v2. Call to set SO_RCVLOWAT for Hyper-V
transport will return EOPNOTSUPP.
Thank You
>
> Thanks,
> -- Dexuan
>