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

[George Dunlap <george.dunlap@citrix.com>]

On 12/13/2017 09:17 AM, Jan Beulich wrote:
>>>> On 12.12.17 at 17:32, <george.dunlap@citrix.com> 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?

 -George

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