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

[Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>]

> 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.

Thanks,
Dan

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