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


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

To: Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH v3] [linux-3.0+ for xen] tmem: self-ballooning and frontswap-selfshrinking
From: Daniel Kiper <dkiper@xxxxxxxxxxxx>
Date: Tue, 5 Jul 2011 19:55:32 +0200
Cc: jeremy@xxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxx, Ian Campbell <Ian.Campbell@xxxxxxxxxx>, Konrad Wilk <konrad.wilk@xxxxxxxxxx>, JBeulich@xxxxxxxxxx, Daniel Kiper <dkiper@xxxxxxxxxxxx>
Delivery-date: Tue, 05 Jul 2011 10:56:30 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <16cb5bf5-a4c4-4379-845a-82668a80b60e@default>
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: <20110704133135.GA6601@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> <16cb5bf5-a4c4-4379-845a-82668a80b60e@default>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.3.28i
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.


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


Xen-devel mailing list