Re: [PATCH v6] memcg: event control at vmpressure.

[Michal Hocko <mhocko@suse.cz>]

On Fri 21-06-13 10:22:34, Minchan Kim wrote:
> On Fri, Jun 21, 2013 at 09:24:38AM +0900, Hyunhee Kim wrote:
> > In the original vmpressure, events are triggered whenever there is a reclaim
> > activity. This becomes overheads to user space module and also increases
> 
> Not true.
> We have lots of filter to not trigger event even if reclaim is going on.
> Your statement would make confuse.

Where is the filter implemented? In the kernel? I do not see any
throttling in the current mm tree.

> > power consumption if there is somebody to listen to it. This patch provides
> > options to trigger events only when the pressure level changes.
> > This trigger option can be set when registering each event by writing
> > a trigger option, "edge" or "always", next to the string of levels.
> > "edge" means that the event is triggered only when the pressure level is changed.
> > "always" means that events are triggered whenever there is a reclaim process.
>                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                                                   Not true, either.

Is this about vmpressure_win? But I agree that this could be more
specific. Something like "`Always' trigger option will signal all events
while `edge' option will trigger only events when the level changes."

> > To keep backward compatibility, "always" is set by default if nothing is input
> > as an option. Each event can have different option. For example,
> > "low" level uses "always" trigger option to see reclaim activity at user space
> > while "medium"/"critical" uses "edge" to do an important job
> > like killing tasks only once.
> 
> Question.
> 
> 1. user: set critical edge
> 2. kernel: memory is tight and trigger event with critical
> 3. user: kill a program when he receives a event
> 4. kernel: memory is very tight again and want to trigger a event
>    with critical but fail because last_level was critical and it was edge.
> 
> Right?

yes, this is the risk of the edge triggering and the user has to be
prepared for that. I still think that it makes some sense to have the
two modes.
 
> > @@ -823,7 +831,7 @@ Test:
> >     # cd /sys/fs/cgroup/memory/
> >     # mkdir foo
> >     # cd foo
> > -   # cgroup_event_listener memory.pressure_level low &
> > +   # cgroup_event_listener memory.pressure_level low edge &
> >     # echo 8000000 > memory.limit_in_bytes
> >     # echo 8000000 > memory.memsw.limit_in_bytes
> >     # echo $$ > tasks
> > diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> > index 736a601..a08252e 100644
> > --- a/mm/vmpressure.c
> > +++ b/mm/vmpressure.c
> > @@ -137,6 +137,8 @@ static enum vmpressure_levels vmpressure_calc_level(unsigned long scanned,
> >  struct vmpressure_event {
> >  	struct eventfd_ctx *efd;
> >  	enum vmpressure_levels level;
> > +	int last_level;
> 
> int? but level is enum vmpressure_levels?

good catch
 
> > +	bool edge_trigger;
> >  	struct list_head node;
> >  };
> >  
> > @@ -153,11 +155,14 @@ static bool vmpressure_event(struct vmpressure *vmpr,
> >  
> >  	list_for_each_entry(ev, &vmpr->events, node) {
> >  		if (level >= ev->level) {
> > +			if (ev->edge_trigger && level == ev->last_level)
> > +				continue;
> > +
> >  			eventfd_signal(ev->efd, 1);
> >  			signalled = true;
> >  		}
> > +		ev->last_level = level;
> >  	}
> > -
> 
> Unnecessary change.
> 
> >  	mutex_unlock(&vmpr->events_lock);
> >  
> >  	return signalled;
> > @@ -290,9 +295,11 @@ void vmpressure_prio(gfp_t gfp, struct mem_cgroup *memcg, int prio)
> >   *
> >   * This function associates eventfd context with the vmpressure
> >   * infrastructure, so that the notifications will be delivered to the
> > - * @eventfd. The @args parameter is a string that denotes pressure level
> > + * @eventfd. The @args parameters are a string that denotes pressure level
> >   * threshold (one of vmpressure_str_levels, i.e. "low", "medium", or
> > - * "critical").
> > + * "critical") and a trigger option that decides whether events are triggered
> > + * continuously or only on edge ("always" or "edge" if "edge", events
> > + * are triggered when the pressure level changes.
> >   *
> >   * This function should not be used directly, just pass it to (struct
> >   * cftype).register_event, and then cgroup core will handle everything by
> > @@ -303,22 +310,43 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
> >  {
> >  	struct vmpressure *vmpr = cg_to_vmpressure(cg);
> >  	struct vmpressure_event *ev;
> > +	char *strlevel, *strtrigger;
> >  	int level;
> > +	bool trigger;
> 
> What trigger?
> Would be better to use "bool egde" instead?

yes

> > +
> > +	strlevel = args;
> > +	strtrigger = strchr(args, ' ');
> > +
> > +	if (strtrigger) {
> > +		*strtrigger = '\0';
> > +		strtrigger++;
> > +	}
> >  
> >  	for (level = 0; level < VMPRESSURE_NUM_LEVELS; level++) {
> > -		if (!strcmp(vmpressure_str_levels[level], args))
> > +		if (!strcmp(vmpressure_str_levels[level], strlevel))
> >  			break;
> >  	}
> >  
> >  	if (level >= VMPRESSURE_NUM_LEVELS)
> >  		return -EINVAL;
> >  
> > +	if (strtrigger == NULL)
> > +		trigger = false;
> > +	else if (!strcmp(strtrigger, "always"))
> > +		trigger = false;
> > +	else if (!strcmp(strtrigger, "edge"))
> > +		trigger = true;
> > +	else
> > +		return -EINVAL;
> > +
> >  	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
> >  	if (!ev)
> >  		return -ENOMEM;
> >  
> >  	ev->efd = eventfd;
> >  	ev->level = level;
> > +	ev->last_level = -1;
> 
> VMPRESSURE_NONE is better?
 
Yes
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>