From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 576D9C433F5 for ; Tue, 7 Sep 2021 13:48:05 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E8571610D0 for ; Tue, 7 Sep 2021 13:48:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E8571610D0 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:35204 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mNbSG-0005XX-0d for qemu-devel@archiver.kernel.org; Tue, 07 Sep 2021 09:48:04 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:42510) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mNb2e-0007sY-FT for qemu-devel@nongnu.org; Tue, 07 Sep 2021 09:21:36 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:27806) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mNb2c-0005Vs-LN for qemu-devel@nongnu.org; Tue, 07 Sep 2021 09:21:36 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1631020893; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=kmGcQ8ycBbHI/lCJq6QqUju6yN1qIo5hni6cMUlhaQY=; b=brD+b/hSyy2/J+VZbZMhOm/Cvv1OiaS/9f3RRULbsGOe3bMYdXF/9fBjrMJZThmGTYt+TC t3/TG/urcSGpxyI0yEv7LIHhAgn1RfRKFJgMM4pIQNMe/Yvm2CJEVw38INmV64e552iIo6 Dk6XXw0TRXChv+yxfO6XlzNZV3Fd4FA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-519-ZLSLH3W9MoCsvEKN6DA_0A-1; Tue, 07 Sep 2021 09:21:27 -0400 X-MC-Unique: ZLSLH3W9MoCsvEKN6DA_0A-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 12B96107ACC7; Tue, 7 Sep 2021 13:21:26 +0000 (UTC) Received: from localhost (unknown [10.39.194.246]) by smtp.corp.redhat.com (Postfix) with ESMTP id AFE045D9DE; Tue, 7 Sep 2021 13:21:20 +0000 (UTC) Date: Tue, 7 Sep 2021 14:21:19 +0100 From: Stefan Hajnoczi To: John Johnson Subject: Re: [PATCH RFC v2 04/16] vfio-user: connect vfio proxy to remote server Message-ID: References: <924FF1F2-E7AF-4044-B5A4-94A2F1504110@oracle.com> MIME-Version: 1.0 In-Reply-To: <924FF1F2-E7AF-4044-B5A4-94A2F1504110@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=stefanha@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Gk6BKDPR5tvLrA2F" Content-Disposition: inline Received-SPF: pass client-ip=216.205.24.124; envelope-from=stefanha@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -31 X-Spam_score: -3.2 X-Spam_bar: --- X-Spam_report: (-3.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.391, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Elena Ufimtseva , Jag Raman , Swapnil Ingle , John Levon , QEMU Devel Mailing List , Alex Williamson , "thanos.makatos@nutanix.com" Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --Gk6BKDPR5tvLrA2F Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Aug 30, 2021 at 03:00:37AM +0000, John Johnson wrote: > > On Aug 24, 2021, at 7:15 AM, Stefan Hajnoczi wrot= e: > > On Mon, Aug 16, 2021 at 09:42:37AM -0700, Elena Ufimtseva wrote: > >> + proxy->ioc =3D ioc; > >> + proxy->flags =3D VFIO_PROXY_CLIENT; > >> + proxy->state =3D VFIO_PROXY_CONNECTED; > >> + qemu_cond_init(&proxy->close_cv); > >> + > >> + if (vfio_user_iothread =3D=3D NULL) { > >> + vfio_user_iothread =3D iothread_create("VFIO user", errp); > >> + } > >=20 > > Why is a dedicated IOThread needed for VFIO user? > >=20 >=20 > =09It seemed the best model for inbound message processing. Most message= s > are replies, so the receiver will either signal a thread waiting the repl= y or > report any errors from the server if there is no waiter. None of this re= quires > the BQL. >=20 > =09If the message is a request - which currently only happens if device > DMA targets guest memory that wasn=E2=80=99t mmap()d by QEMU or if the = =E2=80=99secure-dma=E2=80=99 > option is used - then the receiver can then acquire BQL so it can call th= e > VFIO code to handle the request. QEMU is generally event-driven and the APIs are designed for that style. Threads in QEMU are there for parallelism or resource control, everything else is event-driven. It's not clear to me if there is a reason why the message processing must be done in a separate thread or whether it is just done this way because the code was written in multi-threaded style instead of event-driven style. You mentioned other threads waiting for replies. Which threads are they? > > Please use true. '1' is shorter but it's less obvious to the reader (I > > had to jump to the definition to check whether this field was bool or > > int). > >=20 >=20 > =09I assume this is also true for the other boolean struct members > I=E2=80=99ve added. Yes, please. QEMU uses bool and true/false. >=20 >=20 > >> + aio_bh_schedule_oneshot(iothread_get_aio_context(vfio_user_iothre= ad), > >> + vfio_user_cb, proxy); > >> + > >> + /* drop locks so the iothread can make progress */ > >> + qemu_mutex_unlock_iothread(); > >=20 > > Why does the BQL needs to be dropped so vfio_user_iothread can make > > progress? > >=20 >=20 > =09See above. Acquiring BQL by the iothread is rare, but I have to > handle the case where a disconnect is concurrent with an incoming request > message that is waiting for the BQL. See the proxy state check after BQL > is acquired in vfio_user_recv() Here is how this code looks when written using coroutines (this is from nbd/server.c): static coroutine_fn void nbd_trip(void *opaque) { ... req =3D nbd_request_get(client); ret =3D nbd_co_receive_request(req, &request, &local_err); client->recv_coroutine =3D NULL; =20 if (client->closing) { /* * The client may be closed when we are blocked in * nbd_co_receive_request() */ goto done; } It's the same check. The code is inverted: the server reads the next request using nbd_co_receive_request() (this coroutine function can yield while waiting for data on the socket). This way the network communication code doesn't need to know how messages will by processed by the client or server. There is no need for if (isreply) { qemu_cond_signal(&reply->cv); } else { proxy->request(proxy->reqarg, buf, &reqfds); }. The callbacks and threads aren't hardcoded into the network communication code. This goes back to the question earlier about why a dedicated thread is necessary here. I suggest writing the network communication code using coroutines. That way the code is easier to read (no callbacks or thread synchronization), there are fewer thread-safety issues to worry about, and users or management tools don't need to know about additional threads (e.g. CPU/NUMA affinity). Stefan --Gk6BKDPR5tvLrA2F Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEhpWov9P5fNqsNXdanKSrs4Grc8gFAmE3Z08ACgkQnKSrs4Gr c8iIgwgAu8PEGzWjsCuoiH3QWbJQ76hOkBABUr9tcbzZYesMaMzI1F46WLZMsNru fLU5ha0UA0RVqyb91O2M9IrNSXF6XL4pBssZP77EshkQSoiXGkU7WEbiXhCxCavw t35Ozbz1jI6bU5lPrB5zhQENNyRrfcHj9h+99n68SFLMSGPr9l4DZfta3lMwfKX8 b1dse9OGavvjqwejvFh9udbLAJEnSjKq7SNztb1dfy6cXHaizJNfx5uuV20xysz6 m6Zo1QFacSeFGCVKXLZQAEiNJqfIbIYUkbkHrHL5IijAAn8ot1AK9CYrJRymVKD7 wT1G08g7FvbmYewRc/RQ1wLR1NhMRQ== =Nmjv -----END PGP SIGNATURE----- --Gk6BKDPR5tvLrA2F--