[Xen-devel] RE: [PATCH v3] [linux-3.0+ for xen] tmem: self-ballooning and frontswap-selfshrinking

[Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>]

> From: Daniel Kiper [mailto:dkiper@xxxxxxxxxxxx]
> Subject: Re: [PATCH v3] [linux-3.0+ for xen] tmem: self-ballooning and 
> frontswap-selfshrinking
> 
> On Wed, Jun 29, 2011 at 01:15:24PM -0700, Dan Magenheimer wrote:
> > Hi Konrad (and other reviewers) --
> 
> Sorry for late reply, however, I am very busy now.

Hi Daniel --

Thanks for taking the time to reply.  I've made some of the changes
you suggested and disagree with a couple of others.  See below.

Konrad, I will prepare a v4 and a git tree, but if you have already
pulled, the changes for v4 are all syntactic and have no
functional impact.
 
> > +#ifdef CONFIG_FRONTSWAP
> > +#include <linux/frontswap.h>
> > +#endif
> 
> Please move this condition to linux/frontswap.h and include
> this header file (linux/frontswap.h) unconditionally.

Sorry, this resolves a chicken-and-egg problem as it is.  If
the frontswap patch is not present, there is no file called
include/linux/frontswap.h.  The ifdef can be removed later
when we are sure the frontswap patch is upstream.

> > +static bool __initdata use_selfballooning = true;
> 
> static bool use_selfballooning __initdata = true;
> 
> Please look into include/linux/init.h for details.

OK, there's lots of examples that do it the other way
throughout the kernel, but your reference looks like
it should be authoritative so I made both changes.
 
> > +static unsigned int selfballoon_downhysteresis __read_mostly;
> > +static unsigned int selfballoon_uphysteresis __read_mostly;
> > +
> > +/* in HZ, controls frequency of worker invocation */
> > +static unsigned int selfballoon_interval __read_mostly;
> 
> Could you create a struct selfballoon ???
> I think it will be more readable.

Hmmmm... I guess I disagree.  A struct is useful if, for example,
you are going to pass a reference to a group of variables.  These
are all static and all start with the same prefix (due to Jeremy's
input some time ago that statics should still have unique names
for debug purposes), so I don't think grouping them will make
it more readable.  Maybe you are just used to seeing the
struct in balloon.c?

(same for the other similar comment)
 
> > +   xen_selfballooning_enabled = !!memparse(buf, &endchar);
> 
> Replace memparse() by strict_strtoul().

Again, there are many examples in the kernel that use memparse,
but it appears that newer code does use strict_strtoul,
so I made the changes throughout.

> > +   if (!was_enabled && !xen_selfballooning_enabled &&
> > +                                   frontswap_selfshrinking)
> > +           schedule_delayed_work(&selfballoon_worker,
> > +                   selfballoon_interval * HZ);
> 
> I think it is not worth to wrap lines which only have langht
> slightly above 80 characters limit. In this case two lines
> are more readable.

While I would tend to agree, if checkpatch doesn't like it,
someone is going to complain so I'd rather ensure the 80
character limit is preserved.

> > +struct sys_device;
> > +#ifdef CONFIG_XEN_SELFBALLOONING
> > +extern int register_xen_selfballooning(struct sys_device *sysdev);
> > +#else
> > +static inline int register_xen_selfballooning(struct sys_device *sysdev)
> > +{
> > +   return -1;
> 
> return -ENOSYS;
> 
> or
> 
> return 0;

This is a bit of a nit, since the return value (like all the
other register functions) is ignored, but I made the change
anyway.
 
Thanks again for looking it over, Daniel!

Dan

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