[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [V10 PATCH 11/23] PVH xen: support invalid op emulation for PVH



On 10/08/13 03:13, Mukesh Rathor wrote:
On Thu, 8 Aug 2013 09:55:26 +0100
George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:

On 08/08/13 02:49, Mukesh Rathor wrote:
On Wed, 7 Aug 2013 12:29:13 +0100
George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:

On Wed, Jul 24, 2013 at 2:59 AM, Mukesh Rathor
<mukesh.rathor@xxxxxxxxxx> wrote:
This patch supports invalid op emulation for PVH by calling
appropriate copy macros and and HVM function to inject PF.

Signed-off-by: Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
---
   xen/arch/x86/traps.c        |   17 ++++++++++++++---
   xen/include/asm-x86/traps.h |    1 +
Why make this non-static?  No one is using this in this patch.  If
a later patch needs it, you should make it non-static there, so we
can decide at that point if making it non-static is merited or not.
Sigh! Originally, it was that way, but then to keep that patch from
getting too big, it got moved here after few versions. We are making
emulation available for outside the PV, ie, to PVH.
As far as I'm concerned, the size of the patch itself is immaterial;
the only important question, regarding how to break down patches
(just like in breaking down functions), is how easy or difficult it
is to understand the whole thing.

Now it's typically the case that long patches are hard to understand,
and that breaking them down into smaller chunks makes them easier to
read.  But a division like this, where you've moved some random hunk
into a different patch with which it has no logical relation, makes
the series *harder* to understand, not easier.

Additionally, as the series evolves, it makes it difficult to keep
all of the dependencies straight.  Suppose you changed your approach
for that future patch so that you didn't need this public anymore.
You, and all the reviewers, could easily forget about the dependency,
since it's in a separate patch which may have already been classified
as "OK".
But that would happen even if the function was static. Say I make
changes in function for PVH, dont' make it public. Now I forget to
use it, the function has been changed already?

I said "as the patch series evolves" -- that means before it gets applied. If the hunk making it public is in the same patch that it's used, it is much more likely to be noticed.

We make it public to be used by future patch, I'll add which patch is
using it to make it easier to understand.

Why don't you just *move it* to the patch that's actually using it (the last patch in the series, it would seem -- the one which implements the PVH exit handler)? In the time it took you to write this e-mail, you could have moved that one hunk to the other patch 5 times.

  Not making it public makes
possible  another comment -- why the change if it can't be used by
another PVH module anyways. Can't please all reviewers
simultaneously!!!
All my life so far, all reviews are done by one
person per file, and that makes so much sense..... this is hell!!!

Did someone actually say to you, "This patch is too long, make it shorter"?

I'm not asking for the moon, and I'm not trying to grind your gears. I'm just trying to help you write better patches -- ones which are easier to review, and ones which will be easier for people looking back at to figure out what's going on. And more importantly, in comments in the rest of the series, I'm trying to make code which is easier to understand.

This is important enough to me that I'd actually be willing to take your series and reformat it myself, to demonstrate what I'm talking about. It would be a good exercise for me to become familiar with the code; the only problem is that I'm not up to speed with the details of the hardware at this point.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.