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

[Daniel Kiper <dkiper@xxxxxxxxxxxx>]

On Tue, Jul 05, 2011 at 12:38:03PM -0700, Dan Magenheimer wrote:
> Hi Daniel --
>
> Thanks for taking the time to reply again.  Although you didn't
> say it explicitly, I think you are now OK with V4.  True?

Sorry, I forgot about it. If you change(d) what we agreed it is OK.
However, please look below...

> > > > 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.
>
> You prefer the egg-before-the-chicken, I prefer the
> chicken-before-the-egg. :-) This approach demonstrates
> Xen's clear use for frontswap, and allows trees both with
> frontswap (linux-next) and without frontswap (linux-3.0)
> to properly build.

Your soultion introduce code into linux-3.0 which could not
be enabled (compiled and used) and could confuse others for
what it is for if it could not be enabled.

I still think that this patch should be splited into two independent
(to some extent) entities (selfballooning and frontswap). Later one/both
of them could be applied to appriopriate tree introducing only code
which could be enabled and used.

> > 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).
>
> I agree that strict_strtoul is the better of two very similar
> ways of doing a very similar thing in the kernel.  Changed.

Thanks.

> > > 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.
>
> That's not my experience... it seems to be a personal
> preference and some people have an allergic reaction to
> longer-than-80 lines.  So I prefer to err on the side of
> a clean checkpatch.

OK.

Daniel

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