Re: [PATCH v3 1/7] Basic kernel memory functionality for the MemoryController

[Glauber Costa]

> >   #endif
> >
> > -
> > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM
> > +int do_kmem_account __read_mostly = 1;
> > +#else
> > +#define do_kmem_account		0
> > +#endif
>
>
> Hmm, do we really need this boot option ?
>  From my experience to have swap-accounting boot option,
> this scares us ;) I think config is enough.

If no one else wants it, I can remove it. I personally
don't need it, just wanted to follow the convention laid down by swap here.

> >   /*
> >    * Statistics for memory cgroup.
> >    */
> > @@ -270,6 +274,10 @@ struct mem_cgroup {
> >   	 */
> >   	struct res_counter memsw;
> >   	/*
> > +	 * the counter to account for kmem usage.
> > +	 */
> > +	struct res_counter kmem;
> > +	/*
> >   	 * Per cgroup active and inactive list, similar to the
> >   	 * per zone LRU lists.
> >   	 */
> > @@ -321,6 +329,11 @@ struct mem_cgroup {
> >   	 */
> >   	unsigned long 	move_charge_at_immigrate;
> >   	/*
> > +	 * Should kernel memory limits be stabilished independently
> > +	 * from user memory ?
> > +	 */
> > +	int		kmem_independent;
> > +	/*
> >   	 * percpu counter.
> >   	 */
> >   	struct mem_cgroup_stat_cpu *stat;
> > @@ -388,9 +401,14 @@ enum charge_type {
> >   };
> >
> >   /* for encoding cft->private value on file */
> > -#define _MEM			(0)
> > -#define _MEMSWAP		(1)
> > -#define _OOM_TYPE		(2)
> > +
> > +enum mem_type {
> > +	_MEM = 0,
> > +	_MEMSWAP,
> > +	_OOM_TYPE,
> > +	_KMEM,
> > +};
> > +
>
> ok, nice clean up.
>
>
> >   #define MEMFILE_PRIVATE(x, val)	(((x)<<  16) | (val))
> >   #define MEMFILE_TYPE(val)	(((val)>>  16)&  0xffff)
> >   #define MEMFILE_ATTR(val)	((val)&  0xffff)
> > @@ -3943,10 +3961,15 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *mem, bool swap)
> >   	u64 val;
> >
> >   	if (!mem_cgroup_is_root(mem)) {
> > +		val = 0;
> > +		if (!mem->kmem_independent)
> > +			val = res_counter_read_u64(&mem->kmem, RES_USAGE);
>
> >   		if (!swap)
> > -			return res_counter_read_u64(&mem->res, RES_USAGE);
> > +			val += res_counter_read_u64(&mem->res, RES_USAGE);
> >   		else
> > -			return res_counter_read_u64(&mem->memsw, RES_USAGE);
> > +			val += res_counter_read_u64(&mem->memsw, RES_USAGE);
> > +
> > +		return val;
> >   	}
> >
> >   	val = mem_cgroup_recursive_stat(mem, MEM_CGROUP_STAT_CACHE);
> > @@ -3979,6 +4002,10 @@ static u64 mem_cgroup_read(struct cgroup *cont, struct cftype *cft)
> >   		else
> >   			val = res_counter_read_u64(&mem->memsw, name);
> >   		break;
> > +	case _KMEM:
> > +		val = res_counter_read_u64(&mem->kmem, name);
> > +		break;
> > +
> >   	default:
> >   		BUG();
> >   		break;
> > @@ -4756,6 +4783,21 @@ static int mem_cgroup_reset_vmscan_stat(struct cgroup *cgrp,
> >   	return 0;
> >   }
> >
> > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM
> > +static u64 kmem_limit_independent_read(struct cgroup *cont, struct cftype *cft)
> > +{
> > +	return mem_cgroup_from_cont(cont)->kmem_independent;
> > +}
> > +
> > +static int kmem_limit_independent_write(struct cgroup *cont, struct cftype *cft,
> > +					u64 val)
> > +{
> > +	cgroup_lock();
> > +	mem_cgroup_from_cont(cont)->kmem_independent = !!val;
> > +	cgroup_unlock();
>
> Hm. This code allows that parent/child can have different settings.
> Could you add parent-child check as..
>
> "If parent sets use_hierarchy==1, children must have the same kmem_independent value
> with parant's one."
Agree.
> How do you think ? I think a hierarchy must have the same config.
Yes, I think this is reasonable.

> BTW...I don't like naming a little ;)
>
> memory->consolidated/shared/?????_kmem_accounting ?
> Or
> memory->kmem_independent_accounting ?
>
> or some better naming ?

I can go with kmem_independent_accounting if you like, it is fine
by me.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/