| Message ID | 20230215011614.725983-5-amoorthy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add memory fault exits to avoid slow GUP | expand |
On Wed, Feb 15, 2023 at 01:16:10AM +0000, Anish Moorthy wrote: > The upcoming mem_fault_nowait commits will make it so that, when the > relevant cap is enabled, hva_to_pfn will return after calling > hva_to_pfn_fast without ever attempting to pin memory via > hva_to_pfn_slow. > > hva_to_pfn_fast currently just fails for read-only faults. However, > there doesn't seem to be a reason that we can't just try pinning the > page without FOLL_WRITE instead of immediately falling back to slow-GUP. > This commit implements that behavior. > > Suggested-by: James Houghton <jthoughton@google.com> > Signed-off-by: Anish Moorthy <amoorthy@google.com> > --- > virt/kvm/kvm_main.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index d255964ec331e..dae5f48151032 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2479,7 +2479,7 @@ static inline int check_user_page_hwpoison(unsigned long addr) > } > > /* > - * The fast path to get the writable pfn which will be stored in @pfn, > + * The fast path to get the pfn which will be stored in @pfn, > * true indicates success, otherwise false is returned. It's also the > * only part that runs if we can in atomic context. > */ > @@ -2487,16 +2487,18 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault, > bool *writable, kvm_pfn_t *pfn) > { > struct page *page[1]; > + bool found_by_fast_gup = > + get_user_page_fast_only( > + addr, > + /* > + * Fast pin a writable pfn only if it is a write fault request > + * or the caller allows to map a writable pfn for a read fault > + * request. > + */ > + (write_fault || writable) ? FOLL_WRITE : 0, > + page); > > - /* > - * Fast pin a writable pfn only if it is a write fault request > - * or the caller allows to map a writable pfn for a read fault > - * request. > - */ > - if (!(write_fault || writable)) > - return false; > - > - if (get_user_page_fast_only(addr, FOLL_WRITE, page)) { > + if (found_by_fast_gup) { You could have a smaller diff (and arrive at something more readable) using a local for the gup flags: diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 9c60384b5ae0..57f92ff3728a 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2494,6 +2494,7 @@ static inline int check_user_page_hwpoison(unsigned long addr) static bool hva_to_pfn_fast(unsigned long addr, bool write_fault, bool *writable, kvm_pfn_t *pfn) { + unsigned int gup_flags; struct page *page[1]; /* @@ -2501,10 +2502,9 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault, * or the caller allows to map a writable pfn for a read fault * request. */ - if (!(write_fault || writable)) - return false; + gup_flags = (write_fault || writable) ? FOLL_WRITE : 0; - if (get_user_page_fast_only(addr, FOLL_WRITE, page)) { + if (get_user_page_fast_only(addr, gup_flags, page)) { *pfn = page_to_pfn(page[0]); if (writable)
On Wed, Feb 15, 2023, Oliver Upton wrote: > On Wed, Feb 15, 2023 at 01:16:10AM +0000, Anish Moorthy wrote: > > The upcoming mem_fault_nowait commits will make it so that, when the > > relevant cap is enabled, hva_to_pfn will return after calling > > hva_to_pfn_fast without ever attempting to pin memory via > > hva_to_pfn_slow. > > > > hva_to_pfn_fast currently just fails for read-only faults. However, > > there doesn't seem to be a reason that we can't just try pinning the > > page without FOLL_WRITE instead of immediately falling back to slow-GUP. > > This commit implements that behavior. State what the patch does, avoid pronouns, and especially don't have "This commmit" or "This patch" anywhere. From Documentation/process/submitting-patches.rst: Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour. > > Suggested-by: James Houghton <jthoughton@google.com> > > Signed-off-by: Anish Moorthy <amoorthy@google.com> > > --- > > virt/kvm/kvm_main.c | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index d255964ec331e..dae5f48151032 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -2479,7 +2479,7 @@ static inline int check_user_page_hwpoison(unsigned long addr) > > } > > > > /* > > - * The fast path to get the writable pfn which will be stored in @pfn, > > + * The fast path to get the pfn which will be stored in @pfn, > > * true indicates success, otherwise false is returned. It's also the > > * only part that runs if we can in atomic context. > > */ > > @@ -2487,16 +2487,18 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault, > > bool *writable, kvm_pfn_t *pfn) > > { > > struct page *page[1]; > > + bool found_by_fast_gup = > > + get_user_page_fast_only( > > + addr, > > + /* > > + * Fast pin a writable pfn only if it is a write fault request > > + * or the caller allows to map a writable pfn for a read fault > > + * request. > > + */ > > + (write_fault || writable) ? FOLL_WRITE : 0, > > + page); > > > > - /* > > - * Fast pin a writable pfn only if it is a write fault request > > - * or the caller allows to map a writable pfn for a read fault > > - * request. > > - */ > > - if (!(write_fault || writable)) > > - return false; > > - > > - if (get_user_page_fast_only(addr, FOLL_WRITE, page)) { > > + if (found_by_fast_gup) { > > You could have a smaller diff (and arrive at something more readable) Heh, this whole series just screams "google3". :-) Anish, please read through Documentation/process/coding-style.rst and Documentation/process/submitting-patches.rst particularaly the "Describe your changes" and "Style-check your changes" your changes sections. Bonus points if you work through the mostly redundant process/ documentation, e.g. these have supplementary info. Documentation/process/4.Coding.rst Documentation/process/5.Posting.rst
On Wed, Feb 15, 2023 at 1:02 AM Oliver Upton <oliver.upton@linux.dev> wrote: > > You could have a smaller diff (and arrive at something more readable) > using a local for the gup flags: > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 9c60384b5ae0..57f92ff3728a 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2494,6 +2494,7 @@ static inline int check_user_page_hwpoison(unsigned long addr) > static bool hva_to_pfn_fast(unsigned long addr, bool write_fault, > bool *writable, kvm_pfn_t *pfn) > { > + unsigned int gup_flags; > struct page *page[1]; > > /* > @@ -2501,10 +2502,9 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault, > * or the caller allows to map a writable pfn for a read fault > * request. > */ > - if (!(write_fault || writable)) > - return false; > + gup_flags = (write_fault || writable) ? FOLL_WRITE : 0; > > - if (get_user_page_fast_only(addr, FOLL_WRITE, page)) { > + if (get_user_page_fast_only(addr, gup_flags, page)) { > *pfn = page_to_pfn(page[0]); > > if (writable) Good idea, will do. On Wed, Feb 15, 2023 at 9:03 AM Sean Christopherson <seanjc@google.com> wrote: > > State what the patch does, avoid pronouns, and especially don't have "This commmit" > or "This patch" anywhere. From Documentation/process/submitting-patches.rst: > > Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour. > > Heh, this whole series just screams "google3". :-) > > Anish, please read through > > Documentation/process/coding-style.rst > > and > > Documentation/process/submitting-patches.rst > > particularaly the "Describe your changes" and "Style-check your changes" your > changes sections. Bonus points if you work through the mostly redundant process/ > documentation, e.g. these have supplementary info. > > Documentation/process/4.Coding.rst > Documentation/process/5.Posting.rst Thanks for the resources Sean- I'll go through the series and rework the commit messages/documentation appropriately.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index d255964ec331e..dae5f48151032 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2479,7 +2479,7 @@ static inline int check_user_page_hwpoison(unsigned long addr) } /* - * The fast path to get the writable pfn which will be stored in @pfn, + * The fast path to get the pfn which will be stored in @pfn, * true indicates success, otherwise false is returned. It's also the * only part that runs if we can in atomic context. */ @@ -2487,16 +2487,18 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault, bool *writable, kvm_pfn_t *pfn) { struct page *page[1]; + bool found_by_fast_gup = + get_user_page_fast_only( + addr, + /* + * Fast pin a writable pfn only if it is a write fault request + * or the caller allows to map a writable pfn for a read fault + * request. + */ + (write_fault || writable) ? FOLL_WRITE : 0, + page); - /* - * Fast pin a writable pfn only if it is a write fault request - * or the caller allows to map a writable pfn for a read fault - * request. - */ - if (!(write_fault || writable)) - return false; - - if (get_user_page_fast_only(addr, FOLL_WRITE, page)) { + if (found_by_fast_gup) { *pfn = page_to_pfn(page[0]); if (writable)
The upcoming mem_fault_nowait commits will make it so that, when the relevant cap is enabled, hva_to_pfn will return after calling hva_to_pfn_fast without ever attempting to pin memory via hva_to_pfn_slow. hva_to_pfn_fast currently just fails for read-only faults. However, there doesn't seem to be a reason that we can't just try pinning the page without FOLL_WRITE instead of immediately falling back to slow-GUP. This commit implements that behavior. Suggested-by: James Houghton <jthoughton@google.com> Signed-off-by: Anish Moorthy <amoorthy@google.com> --- virt/kvm/kvm_main.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)