Re: [Xen-devel] [PATCH 4/6] x86/shadow: widen reference count

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.12.17 at 11:32, <george.dunlap@xxxxxxxxxx> wrote:
> On 12/13/2017 09:17 AM, Jan Beulich wrote:
>>>>> On 12.12.17 at 17:32, <george.dunlap@xxxxxxxxxx> wrote:
>>>> @@ -82,7 +153,7 @@ struct page_info
>>>>              unsigned long type:5;   /* What kind of shadow is this? */
>>>>              unsigned long pinned:1; /* Is the shadow pinned? */
>>>>              unsigned long head:1;   /* Is this the first page of the 
>>>> shadow? */
>>>> -#define PAGE_SH_REFCOUNT_WIDTH 25
>>>> +#define PAGE_SH_REFCOUNT_WIDTH (PGT_count_width - 7)
>>>>              unsigned long count:PAGE_SH_REFCOUNT_WIDTH; /* Reference 
>>>> count */
>>>>          } sh;
>> 
>> It is this use of PGT_count_width ...
>> 
>>> What's the point of moving this code?  And are there any important changes?
>> 
>> ... which requires the move. I thought that would be clear enough.
> 
> It's a lot less work for the patch author to point out everything that's
> going on than for a reviewer to infer it.
> 
> The point I was reaching about reviewing your emulator patches was that
> as much as possible, the patch author should make the reviewer's job
> simply one of *verification*.  Consider the following changelog:
> 
> "Move the PG[CT]* definitions before the declaration of the page_info
> struct, so that we can use PGT_count_width to define the width of
> inuse.sh.count."
> 
> Now I don't need to figure out that it's simply code motion, or why it
> might be necessary.  I can see that sh.count uses PGT_count_width
> (verifying that the move is necessary), and I know that the movement is
> simply meant to be code motion, so I can verify that there are no other
> changes (if I feel so inclined).
> 
> It's a lot less work for you to write such a paragraph than it is for
> each of the reviewers to independently infer what's going on.  It's
> "clear enough" if you already have filtered out what's merely code
> motion and what's a substantial change -- but sorting out those two
> requires a certain amount of work, which each reviewer will have to do
> independently.
> 
> Note that there is obviously more to review than verification -- once a
> reviewer determined that the patch is doing what it claims, they also
> need to look for other side effects.  But verification is the first
> step, and the less mental effort a reviewer uses to accomplish the first
> step, the more will be available for the next step, and for other patches.
> 
>>> It would be a lot easier to review if you separated code motion from
>>> code changes; but if you don't want to do that, you need to make it
>>> clear what you're doing in your changelog.
>> 
>> I've added
>> 
>> "Note that the first and last hunks of the xen/include/asm-x86/mm.h
>>  change are merely code motion."
>> 
>> to the description; I don't think separating out that change would
>> make review any easier (you either trust the code-motion-only
>> statement, or you want to compare both blocks, regardless of
>> whether this was a separate patch).
> 
> Fair enough regarding separating code motion and changes.  What do you
> think of the paragraph I suggest above?

I can use that one if you think it's better than the one I had
added already. I'm not sure I see much of a difference in
conveyed information.

Jan


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