Message ID | 1622189463-392610-1-git-send-email-jiayu.hu@intel.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3B1B2A0547; Fri, 28 May 2021 03:42:44 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B2C9940150; Fri, 28 May 2021 03:42:43 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mails.dpdk.org (Postfix) with ESMTP id D77A240040 for <dev@dpdk.org>; Fri, 28 May 2021 03:42:42 +0200 (CEST) IronPort-SDR: PSzDxTGIpxKTvp6uoxIcuKCGkz6obFDJniOzXHbHaPAOtAfmuVITkwDvaX0h80GVFTPNt98yP9 YIffQsETgYeg== X-IronPort-AV: E=McAfee;i="6200,9189,9997"; a="266759997" X-IronPort-AV: E=Sophos;i="5.83,228,1616482800"; d="scan'208";a="266759997" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 May 2021 18:42:40 -0700 IronPort-SDR: Rbfxz3OmGfTMYE9wi0aQXsBQFcgPr4FGjV221ZwtfgrJ/8jyImjETzvDbKSrZKK8FbG2ffy9qi /jCQVvEpy2FA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.83,228,1616482800"; d="scan'208";a="477732975" Received: from npg_dpdk_virtio_jiayuhu_07.sh.intel.com ([10.67.118.193]) by orsmga001.jf.intel.com with ESMTP; 27 May 2021 18:42:38 -0700 From: Jiayu Hu <jiayu.hu@intel.com> To: dev@dpdk.org Cc: maxime.coquelin@redhat.com, chenbo.xia@intel.com, yinan.wang@intel.com, Jiayu Hu <jiayu.hu@intel.com> Date: Fri, 28 May 2021 04:11:01 -0400 Message-Id: <1622189463-392610-1-git-send-email-jiayu.hu@intel.com> X-Mailer: git-send-email 2.7.4 Subject: [dpdk-dev] [PATCH 0/2] provide thread unsafe async registration functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Series |
provide thread unsafe async registration functions
|
|
Message
Hu, Jiayu
May 28, 2021, 8:11 a.m. UTC
Lock protection is needed during the vhost notifies the application of device readiness, so the first patch is to add lock protection. After performing locking, existed async vhost registration functions will cause deadlock, as they acquire lock too. So the second patch is to provide unsafe registration functions to support calling within vhost callback functions. Jiayu Hu (2): vhost: fix lock on device readiness notification vhost: add thread unsafe async registration functions doc/guides/prog_guide/vhost_lib.rst | 12 +++ lib/vhost/rte_vhost_async.h | 42 ++++++++++ lib/vhost/version.map | 4 + lib/vhost/vhost.c | 161 +++++++++++++++++++++++++++--------- lib/vhost/vhost_user.c | 5 +- 5 files changed, 180 insertions(+), 44 deletions(-)
Comments
On 5/28/21 10:11 AM, Jiayu Hu wrote: > Lock protection is needed during the vhost notifies the application of > device readiness, so the first patch is to add lock protection. After > performing locking, existed async vhost registration functions will cause > deadlock, as they acquire lock too. So the second patch is to provide > unsafe registration functions to support calling within vhost callback > functions. > > Jiayu Hu (2): > vhost: fix lock on device readiness notification > vhost: add thread unsafe async registration functions > > doc/guides/prog_guide/vhost_lib.rst | 12 +++ > lib/vhost/rte_vhost_async.h | 42 ++++++++++ > lib/vhost/version.map | 4 + > lib/vhost/vhost.c | 161 +++++++++++++++++++++++++++--------- > lib/vhost/vhost_user.c | 5 +- > 5 files changed, 180 insertions(+), 44 deletions(-) >
Sorry, for previous blank reply. On 5/28/21 10:11 AM, Jiayu Hu wrote: > Lock protection is needed during the vhost notifies the application of > device readiness, so the first patch is to add lock protection. After > performing locking, existed async vhost registration functions will cause > deadlock, as they acquire lock too. So the second patch is to provide > unsafe registration functions to support calling within vhost callback > functions. I agree the callback should be always protected, and in that case having a new thread-unsafe API makes sense for async registration. Regarding backport, I'm not sure what we should do. Backporting new API is a no-go, but with only backporting patch 1 async feature will be always broken on 20.11 LTS, right? What do you think? Thanks, Maxime > Jiayu Hu (2): > vhost: fix lock on device readiness notification > vhost: add thread unsafe async registration functions > > doc/guides/prog_guide/vhost_lib.rst | 12 +++ > lib/vhost/rte_vhost_async.h | 42 ++++++++++ > lib/vhost/version.map | 4 + > lib/vhost/vhost.c | 161 +++++++++++++++++++++++++++--------- > lib/vhost/vhost_user.c | 5 +- > 5 files changed, 180 insertions(+), 44 deletions(-) >
Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <mcoqueli@redhat.com> > Sent: Friday, June 4, 2021 3:25 PM > To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org > Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; > Wang, Yinan <yinan.wang@intel.com> > Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions > > Sorry, for previous blank reply. > > On 5/28/21 10:11 AM, Jiayu Hu wrote: > > Lock protection is needed during the vhost notifies the application of > > device readiness, so the first patch is to add lock protection. After > > performing locking, existed async vhost registration functions will cause > > deadlock, as they acquire lock too. So the second patch is to provide > > unsafe registration functions to support calling within vhost callback > > functions. > > I agree the callback should be always protected, and in that case having > a new thread-unsafe API makes sense for async registration. > > Regarding backport, I'm not sure what we should do. > > Backporting new API is a no-go, but with only backporting patch 1 async > feature will be always broken on 20.11 LTS, right? Yes, if only backporting this fix patch to 20.11 LTS, it may break apps who call async registration functions inside vhost callbacks. How about making this patch not a fix, but a new feature? Thanks, Jiayu > > What do you think? > > Thanks, > Maxime > > > Jiayu Hu (2): > > vhost: fix lock on device readiness notification > > vhost: add thread unsafe async registration functions > > > > doc/guides/prog_guide/vhost_lib.rst | 12 +++ > > lib/vhost/rte_vhost_async.h | 42 ++++++++++ > > lib/vhost/version.map | 4 + > > lib/vhost/vhost.c | 161 +++++++++++++++++++++++++++--------- > > lib/vhost/vhost_user.c | 5 +- > > 5 files changed, 180 insertions(+), 44 deletions(-) > >
Hi Jiayu, On 6/7/21 10:07 AM, Hu, Jiayu wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin <mcoqueli@redhat.com> >> Sent: Friday, June 4, 2021 3:25 PM >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; >> Wang, Yinan <yinan.wang@intel.com> >> Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions >> >> Sorry, for previous blank reply. >> >> On 5/28/21 10:11 AM, Jiayu Hu wrote: >>> Lock protection is needed during the vhost notifies the application of >>> device readiness, so the first patch is to add lock protection. After >>> performing locking, existed async vhost registration functions will cause >>> deadlock, as they acquire lock too. So the second patch is to provide >>> unsafe registration functions to support calling within vhost callback >>> functions. >> >> I agree the callback should be always protected, and in that case having >> a new thread-unsafe API makes sense for async registration. >> >> Regarding backport, I'm not sure what we should do. >> >> Backporting new API is a no-go, but with only backporting patch 1 async >> feature will be always broken on 20.11 LTS, right? > > Yes, if only backporting this fix patch to 20.11 LTS, it may break apps who call > async registration functions inside vhost callbacks. > > How about making this patch not a fix, but a new feature? Async will be still broken in v20.11 in this case. Maybe the better thing would be to remove async support in v20.11, as its support was quite limited in that release anyway. Does that make sense? Thanks, Maxime > Thanks, > Jiayu >> >> What do you think? >> >> Thanks, >> Maxime >> >>> Jiayu Hu (2): >>> vhost: fix lock on device readiness notification >>> vhost: add thread unsafe async registration functions >>> >>> doc/guides/prog_guide/vhost_lib.rst | 12 +++ >>> lib/vhost/rte_vhost_async.h | 42 ++++++++++ >>> lib/vhost/version.map | 4 + >>> lib/vhost/vhost.c | 161 +++++++++++++++++++++++++++--------- >>> lib/vhost/vhost_user.c | 5 +- >>> 5 files changed, 180 insertions(+), 44 deletions(-) >>> >
> -----Original Message----- > From: Maxime Coquelin <mcoqueli@redhat.com> > Sent: Monday, June 7, 2021 9:20 PM > To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org > Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; > Wang, Yinan <yinan.wang@intel.com> > Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions > > Hi Jiayu, > > On 6/7/21 10:07 AM, Hu, Jiayu wrote: > > Hi Maxime, > > > >> -----Original Message----- > >> From: Maxime Coquelin <mcoqueli@redhat.com> > >> Sent: Friday, June 4, 2021 3:25 PM > >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org > >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; > >> Wang, Yinan <yinan.wang@intel.com> > >> Subject: Re: [PATCH 0/2] provide thread unsafe async registration > functions > >> > >> Sorry, for previous blank reply. > >> > >> On 5/28/21 10:11 AM, Jiayu Hu wrote: > >>> Lock protection is needed during the vhost notifies the application of > >>> device readiness, so the first patch is to add lock protection. After > >>> performing locking, existed async vhost registration functions will cause > >>> deadlock, as they acquire lock too. So the second patch is to provide > >>> unsafe registration functions to support calling within vhost callback > >>> functions. > >> > >> I agree the callback should be always protected, and in that case having > >> a new thread-unsafe API makes sense for async registration. > >> > >> Regarding backport, I'm not sure what we should do. > >> > >> Backporting new API is a no-go, but with only backporting patch 1 async > >> feature will be always broken on 20.11 LTS, right? > > > > Yes, if only backporting this fix patch to 20.11 LTS, it may break apps who > call > > async registration functions inside vhost callbacks. > > > > How about making this patch not a fix, but a new feature? > > Async will be still broken in v20.11 in this case. > Maybe the better thing would be to remove async support in v20.11, as > its support was quite limited in that release anyway. Does that make > sense? OK, that makes sense to me. Thanks, Jiayu > > Thanks, > Maxime > > > Thanks, > > Jiayu > >> > >> What do you think? > >> > >> Thanks, > >> Maxime > >> > >>> Jiayu Hu (2): > >>> vhost: fix lock on device readiness notification > >>> vhost: add thread unsafe async registration functions > >>> > >>> doc/guides/prog_guide/vhost_lib.rst | 12 +++ > >>> lib/vhost/rte_vhost_async.h | 42 ++++++++++ > >>> lib/vhost/version.map | 4 + > >>> lib/vhost/vhost.c | 161 +++++++++++++++++++++++++++-------- > - > >>> lib/vhost/vhost_user.c | 5 +- > >>> 5 files changed, 180 insertions(+), 44 deletions(-) > >>> > >
Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <mcoqueli@redhat.com> > Sent: Monday, June 7, 2021 9:20 PM > To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org > Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; > Wang, Yinan <yinan.wang@intel.com> > Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions > > Hi Jiayu, > > On 6/7/21 10:07 AM, Hu, Jiayu wrote: > > Hi Maxime, > > > >> -----Original Message----- > >> From: Maxime Coquelin <mcoqueli@redhat.com> > >> Sent: Friday, June 4, 2021 3:25 PM > >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org > >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; > >> Wang, Yinan <yinan.wang@intel.com> > >> Subject: Re: [PATCH 0/2] provide thread unsafe async registration > >> functions > >> > >> Sorry, for previous blank reply. > >> > >> On 5/28/21 10:11 AM, Jiayu Hu wrote: > >>> Lock protection is needed during the vhost notifies the application > >>> of device readiness, so the first patch is to add lock protection. > >>> After performing locking, existed async vhost registration functions > >>> will cause deadlock, as they acquire lock too. So the second patch > >>> is to provide unsafe registration functions to support calling > >>> within vhost callback functions. > >> > >> I agree the callback should be always protected, and in that case > >> having a new thread-unsafe API makes sense for async registration. > >> > >> Regarding backport, I'm not sure what we should do. > >> > >> Backporting new API is a no-go, but with only backporting patch 1 > >> async feature will be always broken on 20.11 LTS, right? > > > > Yes, if only backporting this fix patch to 20.11 LTS, it may break > > apps who call async registration functions inside vhost callbacks. > > > > How about making this patch not a fix, but a new feature? > > Async will be still broken in v20.11 in this case. > Maybe the better thing would be to remove async support in v20.11, as its > support was quite limited in that release anyway. Does that make sense? The code of supporting async vhost are beyond 1000 lines. I am afraid that removing such more code in 20.11 LTS may get objected. Can we note async register/unregister only work in new_/destroy_device, and using them in other vhost callback functions will cause deadlock in 20.11 LTS instead? Does it make sense to you? Thanks, Jiayu > > Thanks, > Maxime > > > Thanks, > > Jiayu > >> > >> What do you think? > >> > >> Thanks, > >> Maxime
Hi Jiayu, On 6/29/21 7:36 AM, Hu, Jiayu wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin <mcoqueli@redhat.com> >> Sent: Monday, June 7, 2021 9:20 PM >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; >> Wang, Yinan <yinan.wang@intel.com> >> Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions >> >> Hi Jiayu, >> >> On 6/7/21 10:07 AM, Hu, Jiayu wrote: >>> Hi Maxime, >>> >>>> -----Original Message----- >>>> From: Maxime Coquelin <mcoqueli@redhat.com> >>>> Sent: Friday, June 4, 2021 3:25 PM >>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org >>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; >>>> Wang, Yinan <yinan.wang@intel.com> >>>> Subject: Re: [PATCH 0/2] provide thread unsafe async registration >>>> functions >>>> >>>> Sorry, for previous blank reply. >>>> >>>> On 5/28/21 10:11 AM, Jiayu Hu wrote: >>>>> Lock protection is needed during the vhost notifies the application >>>>> of device readiness, so the first patch is to add lock protection. >>>>> After performing locking, existed async vhost registration functions >>>>> will cause deadlock, as they acquire lock too. So the second patch >>>>> is to provide unsafe registration functions to support calling >>>>> within vhost callback functions. >>>> >>>> I agree the callback should be always protected, and in that case >>>> having a new thread-unsafe API makes sense for async registration. >>>> >>>> Regarding backport, I'm not sure what we should do. >>>> >>>> Backporting new API is a no-go, but with only backporting patch 1 >>>> async feature will be always broken on 20.11 LTS, right? >>> >>> Yes, if only backporting this fix patch to 20.11 LTS, it may break >>> apps who call async registration functions inside vhost callbacks. >>> >>> How about making this patch not a fix, but a new feature? >> >> Async will be still broken in v20.11 in this case. >> Maybe the better thing would be to remove async support in v20.11, as its >> support was quite limited in that release anyway. Does that make sense? > > The code of supporting async vhost are beyond 1000 lines. I am afraid that > removing such more code in 20.11 LTS may get objected. Can we note async > register/unregister only work in new_/destroy_device, and using them in > other vhost callback functions will cause deadlock in 20.11 LTS instead? > Does it make sense to you? You are right, that removing 1L LoC in LTS might not be the best idea, since it will cause conflicts when doing backports later on. Maybe the best way is to return -1 at async registration time, with logging an error? Thanks, Maxime > Thanks, > Jiayu >> >> Thanks, >> Maxime >> >>> Thanks, >>> Jiayu >>>> >>>> What do you think? >>>> >>>> Thanks, >>>> Maxime >
> -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Thursday, July 1, 2021 11:43 PM > To: Hu, Jiayu <jiayu.hu@intel.com>; Maxime Coquelin > <mcoqueli@redhat.com>; dev@dpdk.org > Cc: Xia, Chenbo <chenbo.xia@intel.com>; Wang, Yinan > <yinan.wang@intel.com>; David Marchand <david.marchand@redhat.com> > Subject: Re: [PATCH 0/2] provide thread unsafe async registration functions > > Hi Jiayu, > > On 6/29/21 7:36 AM, Hu, Jiayu wrote: > > Hi Maxime, > > > >> -----Original Message----- > >> From: Maxime Coquelin <mcoqueli@redhat.com> > >> Sent: Monday, June 7, 2021 9:20 PM > >> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org > >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; > >> Wang, Yinan <yinan.wang@intel.com> > >> Subject: Re: [PATCH 0/2] provide thread unsafe async registration > >> functions > >> > >> Hi Jiayu, > >> > >> On 6/7/21 10:07 AM, Hu, Jiayu wrote: > >>> Hi Maxime, > >>> > >>>> -----Original Message----- > >>>> From: Maxime Coquelin <mcoqueli@redhat.com> > >>>> Sent: Friday, June 4, 2021 3:25 PM > >>>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org > >>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo > <chenbo.xia@intel.com>; > >>>> Wang, Yinan <yinan.wang@intel.com> > >>>> Subject: Re: [PATCH 0/2] provide thread unsafe async registration > >>>> functions > >>>> > >>>> Sorry, for previous blank reply. > >>>> > >>>> On 5/28/21 10:11 AM, Jiayu Hu wrote: > >>>>> Lock protection is needed during the vhost notifies the > >>>>> application of device readiness, so the first patch is to add lock > protection. > >>>>> After performing locking, existed async vhost registration > >>>>> functions will cause deadlock, as they acquire lock too. So the > >>>>> second patch is to provide unsafe registration functions to > >>>>> support calling within vhost callback functions. > >>>> > >>>> I agree the callback should be always protected, and in that case > >>>> having a new thread-unsafe API makes sense for async registration. > >>>> > >>>> Regarding backport, I'm not sure what we should do. > >>>> > >>>> Backporting new API is a no-go, but with only backporting patch 1 > >>>> async feature will be always broken on 20.11 LTS, right? > >>> > >>> Yes, if only backporting this fix patch to 20.11 LTS, it may break > >>> apps who call async registration functions inside vhost callbacks. > >>> > >>> How about making this patch not a fix, but a new feature? > >> > >> Async will be still broken in v20.11 in this case. > >> Maybe the better thing would be to remove async support in v20.11, as > >> its support was quite limited in that release anyway. Does that make > sense? > > > > The code of supporting async vhost are beyond 1000 lines. I am afraid > > that removing such more code in 20.11 LTS may get objected. Can we > > note async register/unregister only work in new_/destroy_device, and > > using them in other vhost callback functions will cause deadlock in 20.11 > LTS instead? > > Does it make sense to you? > > You are right, that removing 1L LoC in LTS might not be the best idea, since it > will cause conflicts when doing backports later on. > > Maybe the best way is to return -1 at async registration time, with logging an > error? OK. Thanks, Jiayu > > Thanks, > Maxime > > > Thanks, > > Jiayu > >> > >> Thanks, > >> Maxime > >> > >>> Thanks, > >>> Jiayu > >>>> > >>>> What do you think? > >>>> > >>>> Thanks, > >>>> Maxime > >