Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vishnu Dasa <vdasa@vmware.com>
To:     Arseniy Krasnov <AVKrasnov@sberdevices.ru>
CC:     Stefano Garzarella <sgarzare@redhat.com>,
        Bryan Tan <bryantan@vmware.com>,
        Pv-drivers <Pv-drivers@vmware.com>,
        "David S. Miller" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        "kys@microsoft.com" <kys@microsoft.com>,
        "haiyangz@microsoft.com" <haiyangz@microsoft.com>,
        "sthemmin@microsoft.com" <sthemmin@microsoft.com>,
        "wei.liu@kernel.org" <wei.liu@kernel.org>,
        Dexuan Cui <decui@microsoft.com>,
        Krasnov Arseniy <oxffffaa@gmail.com>,
        "virtualization@lists.linux-foundation.org" 
        <virtualization@lists.linux-foundation.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        kernel <kernel@sberdevices.ru>
Subject: Re: [RFC PATCH v2 0/9] vsock: updates for SO_RCVLOWAT handling
Thread-Topic: [RFC PATCH v2 0/9] vsock: updates for SO_RCVLOWAT handling
Thread-Index: AQHYobWftGqT6D2PPUyTyofbXSwoca2TTYkAgAfRQoA=
Date:   Tue, 2 Aug 2022 05:31:26 +0000
Message-ID: <B67A3903-3AF5-40D0-9887-F2253F55C7EB@vmware.com>
References: <19e25833-5f5c-f9b9-ac0f-1945ea17638d@sberdevices.ru>
 <20220727123710.pwzy6ag3gavotxda@sgarzare-redhat>
 <d5166d4e-4892-4cdf-df01-4da43b8e269d@sberdevices.ru>
In-Reply-To: <d5166d4e-4892-4cdf-df01-4da43b8e269d@sberdevices.ru>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?TBnWskDXNrROHryx/Ltu2oicStT26GSDF6mHhbENCppR+mDR81bIqoQqEhvn?=
 =?us-ascii?Q?KkPf+eVmRxA35szghV2kBRzjh5l7ITFBIh6BMjXXKsxeLskqjfbCT5enTP5K?=
 =?us-ascii?Q?s9rZ996hOM1DmZ3oxQxa9HEre9f3Z08lVYGlSpLcf0+2VMuQX9BMgR5sGQ+I?=
 =?us-ascii?Q?jmV11nyQwfm2ElKE/YlChqaFdaRw/89CDO8HrwTNqEwuX9VkxnXLk6FNgf6r?=
 =?us-ascii?Q?EUmLQHivQDyNfoEWOIu5IIcBHm4rm1DTkg/Z+JgE+LlO0uHP0S/IX+K5BeHk?=
 =?us-ascii?Q?ZxirQr0XJrsHWVT10uKTpxYkHvKdYaTTcl/yL+Lh/ocmrb6STy4j7Nnlox31?=
 =?us-ascii?Q?7Vukh/L+XyDVdEwBV2WG0qn3yzWVic73zM5nONkP3jb0a+Vb1Kyj75BLWbrI?=
 =?us-ascii?Q?3br+TA5e3Woa9CZUJxYEPGT14052jq6pKl1/dD7eMwKn70Cm0Fbn68eInA1D?=
 =?us-ascii?Q?N9davl5O3TZEKRbm7tPo+ttlXtvxSMPRlor4U8RW5bbqY5bAeRWWj0XsTWeU?=
 =?us-ascii?Q?UZqJLWCvLYmNHzyMOrI9kheOB1jDUSe5yj4uewuywyCYT47CWNUWW+xaJkst?=
 =?us-ascii?Q?KAmY9wh+wOzJTk038xc39QpgNnsbCEWqzOmuiTG/IP5PO6TE7gNkVE86lP+U?=
 =?us-ascii?Q?23tLFmh1NBLqx0u40E+S9eWCgx7VVdBhet7D1kOfBWsEdBN4wYG5b7RpiJFt?=
 =?us-ascii?Q?yXfIDMm2/RMT5izsLvnnZ+LP6JbOHGHEFVRWjcoz1ysuT9UixKCwqluFa8G7?=
 =?us-ascii?Q?2SsDJ3Azl6dV5BXmXdo4FBgOl+j0IhxoIOouaxk/CNN9vCVj6l9cdMYFziNs?=
 =?us-ascii?Q?SA1M6dorvPIyFKAky6EF1ADT3idKFP/fYaekLUTfLk/mhws7FXuMnCzoq/ow?=
 =?us-ascii?Q?IdNFb/o9sKuAskNzkF0UAnKK4vMa9LFao3kE6NiDTyiATPOWGqyREE8qUUTl?=
 =?us-ascii?Q?eR1PoyyqvabPWZPEHhpcVC5huVF/fy/JHbUlOonscmIWODEzwXSPJY2v6wAr?=
 =?us-ascii?Q?r68EYJttBaqIx1SvqQ6JbAig54fjsovD+IY5I5fRxTlnxH6AQCfa79Hn9WRN?=
 =?us-ascii?Q?Ntayj0JEPT0I49vGHL+JIHFvCq8ZTo6Q/JuClxvGQejZ1rPyEfjDk2ncHvkV?=
 =?us-ascii?Q?jmst4b2966AXhr5O74D/T5JST+mDmcttF0Dt6+8ZqUCjLbsZvVRIVEeOuLnt?=
 =?us-ascii?Q?FvLC+YPKpt2FAmGsR8SwqYk3zFQYd9VMQToWlKeGr1WeZZ2ex0InLkKEWhYf?=
 =?us-ascii?Q?CqnDhdEcD9aw8ZrTeo2pIv3hwJma7EF6nquGlMTFcobXrZcBJOrOkKPXtadx?=
 =?us-ascii?Q?I0CjNt2YWs0o1XAaqwV2JroNiInAgQDbyXzJzawdNuGoPzVi+oAU5Z/pPZ/3?=
 =?us-ascii?Q?AMdzoJuyrO5XUHU4sBaMI96yqAtBtD8PIZpNPH+vOeBR9TIKpxQVch496D6t?=
 =?us-ascii?Q?NJAHfW3PfOFEZ2mocOolKC2hhO+3XlA9zCmyuogNmwl1dGLTxkP/5ILlPon3?=
 =?us-ascii?Q?LA+86T36B0BQa8yycF91oA2ihTlf8GnrEr/xf73p6L4r51aXVZwTQTIY6Y+Y?=
 =?us-ascii?Q?P2soqYWefoZf7ay4CMmboi9n0XMLdIjcvn/Vg3j50ZS8jC/+WxABNLFA7Df9?=
 =?us-ascii?Q?nY1gVmRFnBt4OtT4CKAT0y8Lt97jRIkQo7hn59vncwip?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <217F182527510E499A11E1EC6457B6CB@namprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: vmware.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BYAPR05MB3960.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6179d3fa-b5ba-4bd1-3d6b-08da74483eaf
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Aug 2022 05:31:26.4329
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Qk39j9Y6/UjbvS3vTsEBy30ZGPPs3M75nu0c1zOigJHxl458+XtdVf3zehPqOKtjUGNw/qtO1rP4oAa2q9pTAg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR05MB4751
X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Jul 27, 2022, at 11:08 PM, Arseniy Krasnov <AVKrasnov@sberdevices.ru> =
wrote:
>=20
> On 27.07.2022 15:37, Stefano Garzarella wrote:
>> Hi Arseniy,
>>=20
>> On Mon, Jul 25, 2022 at 07:54:05AM +0000, Arseniy Krasnov wrote:
>>> Hello,
>>>=20
>>> This patchset includes some updates for SO_RCVLOWAT:
>>>=20
>>> 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:
>>>=20
>>> "POLLIN Data other than high-priority data may be read without
>>> blocking.
>>> POLLRDNORM Normal data may be read without blocking."
>>>=20
>>> See https://nam04.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2=
Fwww.open-std.org%2Fjtc1%2Fsc22%2Fopen%2Fn4217.pdf&amp;data=3D05%7C01%7Cvda=
sa%40vmware.com%7Cae83621d8709421de14b08da705faa9c%7Cb39138ca3cee4b4aa4d6cd=
83d9dd62f0%7C0%7C1%7C637945853473740235%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4=
wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp=
;sdata=3DNrbycCcVXV9Tz8NRDYBpnDx7KpFF6BZpSRbuhz1IfJ4%3D&amp;reserved=3D0, p=
age 293.
>>>=20
>>> So, we have, that poll() syscall returns POLLIN, but read call will
>>> be blocked.
>>>=20
>>> Also in man page socket(7) i found that:
>>>=20
>>> "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."
>>>=20
>>> 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.
>>>=20
>>> I've added some fixes to af_vsock.c and virtio_transport_common.c,
>>> test is also implemented.
>>>=20
>>> 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.
>>=20
>> Nice, it looks good!
> Thank You!
>>=20
>>>=20
>>> 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.
>>=20
>> I CCed VMCI maintainers to the patch and also to this cover, maybe bette=
r to keep them in the loop for next versions.
>>=20
>> (Jorgen's and Rajesh's emails bounced back, so I'm CCing here only Bryan=
, Vishnu, and pv-drivers@vmware.com)
> Ok, i'll CC them in the next version
>>=20
>>>=20
>>> 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.
>>=20
>> I left a couple of comments in some patches, but it seems to me to be in=
 a good state :-)
>>=20
>> I would just suggest a bit of a re-organization of the series (the patch=
es 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
>>=20
>> What do you think?
> No problem! I think i can wait for reply from VMWare guys before preparin=
g v3

Looks fine to me, especially the VMCI parts.  Please send v3, and we can te=
st it
from VMCI point of view as well.

Thanks,
Vishnu=