Re: [Xen-devel] [PATCH v2] xen/mm.h: add helper function to test-and-clear _PGC_allocated

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <JBeulich@xxxxxxxx>
> Sent: 15 July 2019 11:44
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: JulienGrall <julien.grall@xxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau 
> Monne
> <roger.pau@xxxxxxxxxx>; VolodymyrBabchuk <Volodymyr_Babchuk@xxxxxxxx>; 
> Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Konrad Rzeszutek 
> Wilk
> <konrad.wilk@xxxxxxxxxx>; Tamas K Lengyel <tamas@xxxxxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>; Wei Liu
> <wl@xxxxxxx>
> Subject: Re: [PATCH v2] xen/mm.h: add helper function to test-and-clear 
> _PGC_allocated
> 
> On 15.07.2019 11:50, Paul Durrant wrote:
> >> From: Jan Beulich <JBeulich@xxxxxxxx>
> >> Sent: 15 July 2019 10:24
> >>
> >> On 15.07.2019 11:17, Paul Durrant wrote:
> >>> The _PGC_allocated flag is set on a page when it is assigned to a domain
> >>> along with an initial reference count of at least 1. To clear this
> >>> 'allocation' reference it is necessary to test-and-clear _PGC_allocated 
> >>> and
> >>> then only drop the reference if the test-and-clear succeeds. This is open-
> >>> coded in many places. It is also unsafe to test-and-clear _PGC_allocated
> >>> unless the caller holds an additional reference.
> >>>
> >>> This patch adds a helper function, put_page_alloc_ref(), to replace all 
> >>> the
> >>> open-coded test-and-clear/put_page occurrences and incorporates in that a
> >>> BUG_ON() an additional page reference not being held.
> >>
> >> This last sentence reads somewhat strange to me - are there words
> >> missing and/or mis-ordered?
> >
> > Perhaps it reads better if 'BUG_ON()' is substituted with 'BUG() on'?
> > I just wanted to express that there was a new check in the helper
> > function that the necessary additional reference is held.
> 
> But then still more like "... incorporates in a BUG() on that an
> additional ..."? At which point it imo could as well be "...
> incorporates in a BUG_ON() that an additional ..." (i.e. just a
> word order change from your original sentence). (There's then
> perhaps also an "is" missing later in the sentence.)
> 
> >>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >>
> >> With the commit message aspect clarified
> >
> > I am happy for you to re-word it if you feel it is not clear.
> 
> Well, the problem is that I don't feel well adjusting what a native
> English speaking person has written.

Ok. How about...

"This patch adds a helper function, put_page_alloc_ref(), to replace all the 
open-coded test-and-clear/put_page occurrences. That helper function 
incorporates a check that an additional page reference is held and will BUG() 
if it is not."

?

  Paul

> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel