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=-9.8 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 1C508C433E4 for ; Wed, 15 Jul 2020 09:36:45 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (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 E048C2064C for ; Wed, 15 Jul 2020 09:36:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=citrix.com header.i=@citrix.com header.b="JmWNlT/2" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E048C2064C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=citrix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jvdpo-0002PR-7G; Wed, 15 Jul 2020 09:36:16 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jvdpn-0002Oi-8O for xen-devel@lists.xenproject.org; Wed, 15 Jul 2020 09:36:15 +0000 X-Inumbo-ID: 9fe4c7e7-c67e-11ea-93a9-12813bfff9fa Received: from esa2.hc3370-68.iphmx.com (unknown [216.71.145.153]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id 9fe4c7e7-c67e-11ea-93a9-12813bfff9fa; Wed, 15 Jul 2020 09:36:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=citrix.com; s=securemail; t=1594805775; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=eAVhxkVPJFov0SlG2bMT+jjb/4wuJ2Q5ALt7SUBTOwA=; b=JmWNlT/243wk6jWsyBsJYJ1zqtzOUxH3UcOr//CadZ/hzXiOuYQduXoq u2oQa0EqxOARG3bOIiXzaiWtSi3JbO6o01om/3FcRWfRD6nVwTaiF/Oxy CLIae4B5m/W+Vv5fgXf7/nt4TmOcf8uei3SK5HR6zb+F29w2MqbeVyQaj I=; Authentication-Results: esa2.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none IronPort-SDR: Un/Fyobl/UYAVxynwhwit+fa0ZWnsYnH9le0Mc4fQg0D/61/UYkxcb8ruMmo6HiixFtSpcT1Qi sWXUactz9LAIZ+dG03z/s+Ln1+c+XNc6NH/AMG25Aix2ftrgFxPWdQJobJlIkxLiW/fuBq1SRM nYLhaE4Ys1b3Ak8eS9BddkswWL9yzZveiRaRVOYGYftMw/Ek07trSjIswP5wNKOepmR+rOZeou qj184Zo3XnFquoCxWgmZZ6fpeFl3EEomn23djO+MjcWmkllPQwFLeXh/SFY4rEIBmE6f9wRbkB alE= X-SBRS: 2.7 X-MesageID: 22418574 X-Ironport-Server: esa2.hc3370-68.iphmx.com X-Remote-IP: 162.221.158.21 X-Policy: $RELAYED X-IronPort-AV: E=Sophos;i="5.75,354,1589256000"; d="scan'208";a="22418574" Date: Wed, 15 Jul 2020 11:36:06 +0200 From: Roger Pau =?utf-8?B?TW9ubsOp?= To: =?utf-8?Q?Micha=C5=82_Leszczy=C5=84ski?= Subject: Re: [PATCH v6 01/11] memory: batch processing in acquire_resource() Message-ID: <20200715093606.GU7191@Air-de-Roger> References: <02415890e4e8211513b495228c790e1d16de767f.1594150543.git.michal.leszczynski@cert.pl> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <02415890e4e8211513b495228c790e1d16de767f.1594150543.git.michal.leszczynski@cert.pl> X-ClientProxiedBy: AMSPEX02CAS02.citrite.net (10.69.22.113) To AMSPEX02CL02.citrite.net (10.69.22.126) X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Stefano Stabellini , Julien Grall , Wei Liu , Andrew Cooper , Ian Jackson , George Dunlap , luwei.kang@intel.com, tamas.lengyel@intel.com, Jan Beulich , xen-devel@lists.xenproject.org Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" On Tue, Jul 07, 2020 at 09:39:40PM +0200, Michał Leszczyński wrote: > From: Michal Leszczynski > > Allow to acquire large resources by allowing acquire_resource() > to process items in batches, using hypercall continuation. > > Be aware that this modifies the behavior of acquire_resource > call with frame_list=NULL. While previously it would return > the size of internal array (32), with this patch it returns > the maximal quantity of frames that could be requested at once, > i.e. UINT_MAX >> MEMOP_EXTENT_SHIFT. > > Signed-off-by: Michal Leszczynski > --- FWIW, I think I've also said on a previous version, I would prefer if the changelog between versions is added to each patch, having it on the cover letter is not very helpful as I usually care about specific changes made to each patch. I've just got one comment that needs addressing below. > xen/common/memory.c | 49 ++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 44 insertions(+), 5 deletions(-) > > diff --git a/xen/common/memory.c b/xen/common/memory.c > index 714077c1e5..eb42f883df 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -1046,10 +1046,12 @@ static int acquire_grant_table(struct domain *d, unsigned int id, > } > > static int acquire_resource( > - XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg) > + XEN_GUEST_HANDLE_PARAM(xen_mem_acquire_resource_t) arg, > + unsigned long *start_extent) > { > struct domain *d, *currd = current->domain; > xen_mem_acquire_resource_t xmar; > + uint32_t total_frames; > /* > * The mfn_list and gfn_list (below) arrays are ok on stack for the > * moment since they are small, but if they need to grow in future > @@ -1069,7 +1071,7 @@ static int acquire_resource( > if ( xmar.nr_frames ) > return -EINVAL; > > - xmar.nr_frames = ARRAY_SIZE(mfn_list); > + xmar.nr_frames = UINT_MAX >> MEMOP_EXTENT_SHIFT; > > if ( __copy_field_to_guest(arg, &xmar, nr_frames) ) > return -EFAULT; > @@ -1077,8 +1079,28 @@ static int acquire_resource( > return 0; > } > > + total_frames = xmar.nr_frames; > + > + /* Is the size too large for us to encode a continuation? */ > + if ( unlikely(xmar.nr_frames > (UINT_MAX >> MEMOP_EXTENT_SHIFT)) ) > + return -EINVAL; > + > + if ( *start_extent ) > + { > + /* > + * Check whether start_extent is in bounds, as this > + * value if visible to the calling domain. > + */ > + if ( *start_extent > xmar.nr_frames ) > + return -EINVAL; > + > + xmar.frame += *start_extent; > + xmar.nr_frames -= *start_extent; > + guest_handle_add_offset(xmar.frame_list, *start_extent); > + } > + > if ( xmar.nr_frames > ARRAY_SIZE(mfn_list) ) > - return -E2BIG; > + xmar.nr_frames = ARRAY_SIZE(mfn_list); > > rc = rcu_lock_remote_domain_by_id(xmar.domid, &d); > if ( rc ) > @@ -1135,6 +1157,14 @@ static int acquire_resource( > } > } > > + if ( !rc ) > + { > + *start_extent += xmar.nr_frames; > + > + if ( *start_extent != total_frames ) > + rc = -ERESTART; > + } > + > out: > rcu_unlock_domain(d); > > @@ -1599,8 +1629,17 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > #endif > > case XENMEM_acquire_resource: > - rc = acquire_resource( > - guest_handle_cast(arg, xen_mem_acquire_resource_t)); > + do { > + rc = acquire_resource( > + guest_handle_cast(arg, xen_mem_acquire_resource_t), > + &start_extent); I think it would be interesting from a performance PoV to move the xmar copy_from_guest here, so that each call to acquire_resource in the loop doesn't need to perform a copy from guest. That's more relevant for translated callers, where a copy_from_guest involves a guest page table and a p2m walk. > + > + if ( hypercall_preempt_check() ) You are missing a rc == -ERESTART check here, you don't want to encode a continuation if rc is different than -ERESTART AFAICT. Thanks, Roger.