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/
Home Products Support Community News


RE: [Xen-devel] [PATCH] [linux] xen: tmem: frontswap-tmemonly

To: Konrad Wilk <konrad.wilk@xxxxxxxxxx>
Subject: RE: [Xen-devel] [PATCH] [linux] xen: tmem: frontswap-tmemonly
From: Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>
Date: Thu, 16 Jun 2011 14:17:42 -0700 (PDT)
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, Jeremy Fitzhardinge <jeremy.fitzhardinge@xxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxxxx>, Nikanth Karthikesan <nikanth@xxxxxxxxxx>
Delivery-date: Thu, 16 Jun 2011 14:24:32 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <20110616183849.GA28775@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/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <bc0d3d9a-b9be-4260-b1d1-a6a336e0162c@default 20110616183849.GA28775@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
> From: Konrad Rzeszutek Wilk

Can I add a "Reviewed-by:"?  Or will this get
a S-O-B from you-as-maintainer anyway?
> This is a quite small file.. This is it?

Thanks for the review!  Yes, this file is just a shim
translating kernel-ese to Xen-tmem-ese.

I realize I do need to add a tmem.h file (with one line
of code in it) to surface the exported tmem_enabled.

> This looks like you are using spaces instead of tabs. But it
> could be the editor of mine mugling things up.
>  :
> Hm, not aligned?

These must both be your editor (or maybe my mailer
if you are editing the inline version), they are fine
in the patch and in git.

> This could be just : return (ret == 1) ? 0 : -1;

I'm inclined to spell this out as it is.  IMHO, at least in
this case, clarity trumps brevity.  It is consistent with the
rest of the file too.  If you feel strongly though, let me know.

> > +   if (pool < 0)
> > +           return -1;
> > +   if (ind64 != ind)
> > +           return -1;
> This looks to repeat itself in the previous function. You might want to
> make this a macro. Or a short inline function in the header file

Same here.  I think a macro will obfuscate this code, and only will
remove a few lines.  Again, if you feel strongly...

> > +static int use_frontswap = 1;
> You could save some precious bytes by making this __initdata
> Also the naming is different. You have tmem_enabled. How about
> frontswap_enabled instead?
OK, will fix.

V2 to be posted soon.


Xen-devel mailing list

<Prev in Thread] Current Thread [Next in Thread>