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

[Daniel Kiper <dkiper@xxxxxxxxxxxx>]

On Mon, Jul 04, 2011 at 01:06:33PM -0700, Dan Magenheimer wrote:
> > > +#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.

Hmmm... I think that in this situation
it should be moved to frontswap patch.

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

Thanks.

> > > +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)

I do not fully agree, however, I do not insist on changing that.

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

As I saw it was designed to read memory size from kernel
command line and module options (lib/cmdline.c). It is
mostly used in that context. Additionally, you are using
memparse() for parsing values which are not memory sizes.
It could be misleading. That is why I asked you to
change that to strict_strtoul() (it is generic).

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

Line lengths overlimits are marked as warnings. If they are sane
then kernel developers do not complain.

Daniel

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