Re: [RFC PATCH v2 0/9] vsock: updates for SO_RCVLOWAT handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Arseniy,

On Mon, Jul 25, 2022 at 07:54:05AM +0000, Arseniy Krasnov wrote:
Hello,

This patchset includes some updates for SO_RCVLOWAT:

1) af_vsock:
  During my experiments with zerocopy receive, i found, that in some
  cases, poll() implementation violates POSIX: when socket has non-
  default SO_RCVLOWAT(e.g. not 1), poll() will always set POLLIN and
  POLLRDNORM bits in 'revents' even number of bytes available to read
  on socket is smaller than SO_RCVLOWAT value. In this case,user sees
  POLLIN flag and then tries to read data(for example using  'read()'
  call), but read call will be blocked, because  SO_RCVLOWAT logic is
  supported in dequeue loop in af_vsock.c. But the same time,  POSIX
  requires that:

  "POLLIN     Data other than high-priority data may be read without
              blocking.
   POLLRDNORM Normal data may be read without blocking."

  See https://www.open-std.org/jtc1/sc22/open/n4217.pdf, page 293.

  So, we have, that poll() syscall returns POLLIN, but read call will
  be blocked.

  Also in man page socket(7) i found that:

  "Since Linux 2.6.28, select(2), poll(2), and epoll(7) indicate a
  socket as readable only if at least SO_RCVLOWAT bytes are available."

  I checked TCP callback for poll()(net/ipv4/tcp.c, tcp_poll()), it
  uses SO_RCVLOWAT value to set POLLIN bit, also i've tested TCP with
  this case for TCP socket, it works as POSIX required.

  I've added some fixes to af_vsock.c and virtio_transport_common.c,
  test is also implemented.

2) virtio/vsock:
  It adds some optimization to wake ups, when new data arrived. Now,
  SO_RCVLOWAT is considered before wake up sleepers who wait new data.
  There is no sense, to kick waiter, when number of available bytes
  in socket's queue < SO_RCVLOWAT, because if we wake up reader in
  this case, it will wait for SO_RCVLOWAT data anyway during dequeue,
  or in poll() case, POLLIN/POLLRDNORM bits won't be set, so such
  exit from poll() will be "spurious". This logic is also used in TCP
  sockets.

Nice, it looks good!


3) vmci/vsock:
  Same as 2), but i'm not sure about this changes. Will be very good,
  to get comments from someone who knows this code.

I CCed VMCI maintainers to the patch and also to this cover, maybe better to keep them in the loop for next versions.

(Jorgen's and Rajesh's emails bounced back, so I'm CCing here only Bryan, Vishnu, and pv-drivers@xxxxxxxxxx)


4) Hyper-V:
  As Dexuan Cui mentioned, for Hyper-V transport it is difficult to
  support SO_RCVLOWAT, so he suggested to disable this feature for
  Hyper-V.

I left a couple of comments in some patches, but it seems to me to be in a good state :-)

I would just suggest a bit of a re-organization of the series (the patches are fine, just the order):
  - introduce vsock_set_rcvlowat()
  - disabling it for hv_sock
  - use 'target' in virtio transports
  - use 'target' in vmci transports
  - use sock_rcvlowat in vsock_poll()
    I think is better to pass sock_rcvlowat() as 'target' when the
    transports are already able to use it
  - add vsock_data_ready()
  - use vsock_data_ready() in virtio transports
  - use vsock_data_ready() in vmci transports
  - tests

What do you think?

Thanks,
Stefano




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux