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

[Michal Hocko <mhocko@suse.cz>]

On Mon 17-06-13 20:30:11, Hyunhee Kim wrote:
[...]
> diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> index 736a601..a18fdb3 100644
> --- a/mm/vmpressure.c
> +++ b/mm/vmpressure.c
[...]
> @@ -150,14 +151,16 @@ static bool vmpressure_event(struct vmpressure *vmpr,
>  	level = vmpressure_calc_level(scanned, reclaimed);
>  
>  	mutex_lock(&vmpr->events_lock);
> -
>  	list_for_each_entry(ev, &vmpr->events, node) {
>  		if (level >= ev->level) {
> +			if (ev->edge_trigger && (level == vmpr->last_level

> +				|| level != ev->level))

Hmm, why this differs from the "always" semantic? You do not want to see
lower events? Why?

> +				continue;
>  			eventfd_signal(ev->efd, 1);
>  			signalled = true;
>  		}
>  	}
> -
> +	vmpr->last_level = level;
>  	mutex_unlock(&vmpr->events_lock);

I have already asked in the previous version but there was no answer for
it. So let's try again.

What is the expected semantic when an event is triggered but there is
nobody to consume it?
I am not sure that the current implementation is practical. Say that
last_level is LOW and you just registered your event. Should you see the
first event or not?

I think that last_level should be per-vmpressure_event and the edge
would be defined as the even seen for the first time since registration.

>  	return signalled;
> @@ -290,9 +293,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", only the current
> + * pressure level is 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,10 +308,14 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
>  {
>  	struct vmpressure *vmpr = cg_to_vmpressure(cg);
>  	struct vmpressure_event *ev;
> +	char strlevel[32], strtrigger[32] = "always";
>  	int level;
>  
> +	if ((sscanf(args, "%s %s\n", strlevel, strtrigger) > 2))
> +		return -EINVAL;

Ouch! You should rather not let your users write over your stack, should
you?

> +
>  	for (level = 0; level < VMPRESSURE_NUM_LEVELS; level++) {
> -		if (!strcmp(vmpressure_str_levels[level], args))
> +		if (!strcmp(vmpressure_str_levels[level], strlevel))
>  			break;
>  	}
>  
[...]
-- 
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>