On Mon, 15 Apr 2013 13:58:05 -0700
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 12 Apr 2013 17:11:08 +0900 Daisuke Nishimura <nishimura@xxxxxxxxxxxxxxxxx> wrote:
>
> > > --- a/include/linux/res_counter.h
> > > +++ b/include/linux/res_counter.h
> > > @@ -54,7 +54,7 @@ struct res_counter {
> > > struct res_counter *parent;
> > > };
> > >
> > > -#define RESOURCE_MAX (unsigned long long)LLONG_MAX
> > > +#define RESOURCE_MAX (unsigned long long)ULLONG_MAX
> > >
> >
> > I don't think it's a good idea to change a user-visible value.
>
> The old value was a mistake, surely.
>
I introduced 'RESOURCE_MAX' in commit:c5b947b2, but I just used the
default value of the res_counter.limit. I'm not sure why it had been
initialized to (unsigned long long)LLONG_MAX.
> RESOURCE_MAX shouldn't be in this header file - that is far too general
> a name. I suggest the definition be moved to res_counter.c. And the
> (unsigned long long) cast is surely unneeded if we're to use
> ULLONG_MAX.
>
Hmm, RESOUCE_MAX is now used outside of res_counter.c(e.g. tcp_memcontrol.c).
Adding Glauber Costa to the cc list.
Just changing the name to RES_COUNTER_MAX might be the choice.
> > > /**
> > > * Helpers to interact with userspace
> > > diff --git a/kernel/res_counter.c b/kernel/res_counter.c
> > > index ff55247..6c35310 100644
> > > --- a/kernel/res_counter.c
> > > +++ b/kernel/res_counter.c
> > > @@ -195,6 +195,12 @@ int res_counter_memparse_write_strategy(const char *buf,
> > > if (*end != '\0')
> > > return -EINVAL;
> > >
> > > - *res = PAGE_ALIGN(*res);
> > > + /* Since PAGE_ALIGN is aligning up(the next page boundary),
> > > + * check the left space to avoid overflow to 0. */
> > > + if (RESOURCE_MAX - *res < PAGE_SIZE - 1)
> > > + *res = RESOURCE_MAX;
> > > + else
> > > + *res = PAGE_ALIGN(*res);
> > > +
> >
> > Current interface seems strange because we can set a bigger value than
> > the value which means "unlimited".
>
> I'm not sure what you mean by this?
>
9223372036854775807(LLONG_MAX) means "unlimited" now. But we can set a bigger
value than that.
# cat /cgroup/memory/A/memory.memsw.limit_in_bytes
9223372036854775807
# echo 9223372036854775808 >/cgroup/memory/A/memory.memsw.limit_in_bytes
# cat /cgroup/memory/A/memory.memsw.limit_in_bytes
9223372036854775808
I feel "bigger than unlimited" is strange.
> > So, how about some thing like:
> >
> > if (*res > RESOURCE_MAX)
> > return -EINVAL;
> > if (*res > PAGE_ALIGN(RESOURCE_MAX) - PAGE_SIZE)
> > *res = RESOURCE_MAX;
> > else
> > *res = PAGE_ALIGN(*res);
> >
>
> The first thing I'd do to res_counter_memparse_write_strategy() is to
> rename its second arg to `resp' then add a local called `res'. Because
> that function dereferences res far too often.
>
> Then,
>
> - *res = PAGE_ALIGN(*res);
> if (PAGE_ALIGN(res) >= res)
> res = PAGE_ALIGN(res);
> else
> res = RESOURCE_MAX; /* PAGE_ALIGN wrapped to zero */
>
> *resp = res;
> return 0;
>
>
Good idea :)
Thanks,
Daisuke Nishimura.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>