All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Xu <peterx@redhat.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: kvm@vger.kernel.org, eric.auger@redhat.com, kwankhede@nvidia.com,
	linux-kernel@vger.kernel.org, slp@redhat.com
Subject: Re: [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external()
Date: Mon, 17 Apr 2017 14:54:21 +0800	[thread overview]
Message-ID: <20170417065420.GD16703@pxdev.xzpeter.org> (raw)
In-Reply-To: <20170417014239.25866.42333.stgit@gimli.home>

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 <kwankhede@nvidia.com>
> Suggested-by: Peter Xu <peterx@redhat.com>

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 <peterx@redhat.com>

Thanks!

-- 
Peter Xu

  reply	other threads:[~2017-04-17  6:54 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-17  1:42 [PATCH v4 0/2] vfio/type1: Synchronous locked page accounting Alex Williamson
2017-04-17  1:42 ` [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue Alex Williamson
2017-04-17  6:47   ` Peter Xu
2017-04-17 14:32     ` Alex Williamson
2017-04-17 19:05       ` Kirti Wankhede
2017-04-17 19:19         ` Alex Williamson
2017-04-17 19:32           ` Kirti Wankhede
2017-04-17 21:32             ` Alex Williamson
2017-04-18  2:54               ` Peter Xu
2017-04-18 18:21                 ` Kirti Wankhede
2017-04-17  1:42 ` [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() Alex Williamson
2017-04-17  6:54   ` Peter Xu [this message]
2017-04-17 17:20     ` Alex Williamson
2017-04-17 19:16   ` Kirti Wankhede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170417065420.GD16703@pxdev.xzpeter.org \
    --to=peterx@redhat.com \
    --cc=alex.williamson@redhat.com \
    --cc=eric.auger@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=kwankhede@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=slp@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.