2013/6/17 Michal Hocko <mhocko@xxxxxxx>:
> 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?
Yes, I didn't want to see every lower level events whenever the higher
level event occurs because the higher event signal implies that the
lower memory situation also occurs. For example, if CRITICAL event
occurs, it means that LOW and MEDIUM also occur. So, I think that
triggering these lower level events are redundant. And, some users
don't want to see this every event. But, I think that if I don't want
to see lower events, I should add another option. Currently, as you
mentioned, for edge_trigger option, I'll remove "level != ev->level"
part.
>
>> + 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.
Right. The current implementation could not cover those situations. As
you mentioned, I think that this could be solved by having last_level
per vmpressure_event (after removing "level != ev->level"). If
last_level of each event is set to valid level only after the first
event is signaled, we cannot miss the first signal even when an event
is registered in the middle of runtime.
>
>> 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?
>
Sorry, this should be fixed in the next patch.
>> +
>> 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@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
Thanks,
Hyunhee Kim.
--
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>