WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH] wrong accounting in direct_remap_pfn_range

To: Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] wrong accounting in direct_remap_pfn_range
From: Steven Rostedt <srostedt@xxxxxxxxxx>
Date: Wed, 30 Aug 2006 21:35:31 -0400
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, quintela@xxxxxxxxxx
Delivery-date: Wed, 30 Aug 2006 18:35:08 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <C11BF47E.1939%Keir.Fraser@xxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <C11BF47E.1939%Keir.Fraser@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Thunderbird 1.5.0.4 (X11/20060614)
Keir Fraser wrote:


On 31/8/06 2:03 am, "Steven Rostedt" <srostedt@xxxxxxxxxx> wrote:

It's not really missing. We could have a size==0 check *or* we can have the
v!=u check. We don't need both and I think the latter is more obviously
correct, as the test is closer to the code that it 'protects'. Also it's a
fairly idiomatic way of generating and flushing batches of work.

So what is really wrong with this code?  Or is the flushes need even on
size == 0?

This patch is fine. But it's no more correct than the current version of the
code because there is no bug. I don't think your version is particularly
clearer.


You're right that the original version was not a bug, which I admittedly thought it was.

As for clarity, I'll argue that my version is clearer. Just because it shows exactly that nothing needs to be done when size is zero, instead having a check for (v != u) that is only true when we have a zero size.

Usually, when I have a condition of that sort that comes after a loop incrementing v and lets v return back to u, I have that condition need to be done if a) the loop wasn't entered (as in this case), or b) the condition in the loop wasn't done at the end (which isn't the case here).

Hence, the reason that I thought it was a bug. Nowhere is there a comment that says /* don't do anything if size was 0 */.

Now enough about clarity. By the off chance that size would be zero (which I still don't know when that would be), we waste time with flushing caches and the TLB, not to mention wasted time getting a memory page and freeing it.

But this is all moot anyway, since there is no bug being fixed. But as for submitting to mainline, I've submitted enough patches to know that Andrew Morton actually cares about readability and optimizations (even on the not so often path if it doesn't hurt anything else). And I have a tough time seeing Andrew (or others) think that

  v = u;
  for (i=0; i < size; ...) {
     if (v - u/ ...) {
        ...
        v = u;
        ...
     }
     ...
     v++;
     ...
  }
  if (v != u) { ... }

is more readable and clearer than

  if (!size) return 0;

  for (...) {
    ...
  }
  /* no if needed */
  ....


But that's just my opinion.  Take it for what it's worth.

-- Steve

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel