From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() Date: Mon, 17 Apr 2017 14:54:21 +0800 Message-ID: <20170417065420.GD16703@pxdev.xzpeter.org> References: <20170417014142.25866.16852.stgit@gimli.home> <20170417014239.25866.42333.stgit@gimli.home> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: kvm@vger.kernel.org, eric.auger@redhat.com, kwankhede@nvidia.com, linux-kernel@vger.kernel.org, slp@redhat.com To: Alex Williamson Return-path: Received: from mx1.redhat.com ([209.132.183.28]:37756 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932813AbdDQGyd (ORCPT ); Mon, 17 Apr 2017 02:54:33 -0400 Content-Disposition: inline In-Reply-To: <20170417014239.25866.42333.stgit@gimli.home> Sender: kvm-owner@vger.kernel.org List-ID: On Sun, Apr 16, 2017 at 07:42:39PM -0600, Alex Williamson wrote: > With vfio_lock_acct() testing the locked memory limit under mmap_sem, > it's redundant to do it here for a single page. We can also reorder > our tests such that we can avoid testing for reserved pages if we're > not doing accounting, and test the process CAP_IPC_LOCK only if we > are doing accounting. Finally, this function oddly returns 1 on > success. Update to return zero on success, -errno on error. Since > the function only pins a single page, there's no need to return the > number of pages pinned. > > N.B. vfio_pin_pages_remote() can pin a large contiguous range of pages > before calling vfio_lock_acct(). If we were to similarly remove the > extra test there, a user could temporarily pin far more pages than > they're allowed. > > Suggested-by: Kirti Wankhede > Suggested-by: Peter Xu Maybe this suggested-by honor should be for Kirti only? :) For the patch, I think it's good to me as long as we have the accounting check in vfio_lock_acct() which is just introduced in previous patch, so: Reviewed-by: Peter Xu Thanks! -- Peter Xu