2013/6/19 Michal Hocko <mhocko@xxxxxxx>:
> OK, this looks much better. Few nitpicks bellow.
>
> On Wed 19-06-13 20:31:16, 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
>> 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.
>> 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.
>>
>> Signed-off-by: Hyunhee Kim <hyunhee.kim@xxxxxxxxxxx>
>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>> ---
>> Documentation/cgroups/memory.txt | 12 ++++++++++--
>> mm/vmpressure.c | 32 ++++++++++++++++++++++++++++----
>> 2 files changed, 38 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
>> index ddf4f93..181a11f 100644
>> --- a/Documentation/cgroups/memory.txt
>> +++ b/Documentation/cgroups/memory.txt
>> @@ -791,6 +791,13 @@ way to trigger. Applications should do whatever they can to help the
>> system. It might be too late to consult with vmstat or any other
>> statistics, so it's advisable to take an immediate action.
>>
>> +Events can be triggered whenever there is a reclaim activity or
>> +only when the pressure level changes. Trigger option is decided
>> +by writing it next to level. The event whose trigger option is "always"
>> +is triggered whenever there is a reclaim process. If "edge" is set,
>> +the event is triggered only when the level is changed.
>> +If the trigger option is not set, "always" is set by default.
>> +
>
> I would move this information bellow where the usage is described.
Ok, I'll move this bellow the usage description.
>
>> The events are propagated upward until the event is handled, i.e. the
>> events are not pass-through. Here is what this means: for example you have
>> three cgroups: A->B->C. Now you set up an event listener on cgroups A, B
> [...]
>> diff --git a/mm/vmpressure.c b/mm/vmpressure.c
>> index 736a601..4f676b8 100644
>> --- a/mm/vmpressure.c
>> +++ b/mm/vmpressure.c
> [...]
>> @@ -303,10 +310,21 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
>> {
>> struct vmpressure *vmpr = cg_to_vmpressure(cg);
>> struct vmpressure_event *ev;
>> + char *strlevel = NULL, *strtrigger = NULL, *p = NULL;
>> int level;
>>
>> + p = strchr(args, ' ');
>> +
>> + if (p) {
>> + strtrigger = p + 1;
>> + *p = '\0';
>> + strlevel = (char *)args;
>> + } else {
>> + strlevel = (char *)args;
>> + }
>> +
>
> This is a total nit but this can be further simplified.
> strlevel = args;
> strtrigger = strchr(args, ' ');
> if (strtrigger) {
> *strtrigger = '\0';
> strtrigger++;
> }
> I would still rather see using sscanf but that is just a matter of taste
> I guess.
Thanks for the comments. This looks better and more simple.
>
>> for (level = 0; level < VMPRESSURE_NUM_LEVELS; level++) {
>> - if (!strcmp(vmpressure_str_levels[level], args))
>> + if (!strcmp(vmpressure_str_levels[level], strlevel))
>> break;
>> }
>>
>> @@ -319,6 +337,12 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
>>
>> ev->efd = eventfd;
>> ev->level = level;
>> + ev->last_level = -1;
>> +
>> + if (strtrigger && !strcmp(strtrigger, "edge"))
>> + ev->edge_trigger = true;
>> + else
>> + ev->edge_trigger = false;
>
> I guess it would be more appropriate to return EINVAL if the trigger is
> neither always nor edge because we might end up with abuses where
> somebody start relying on "foo" implying "always".
>
> The history tells that user interface should be really careful about not
> allowing "undocumented but happen to work" behavior.
Yes, right. I overlooked the case when somebody inputs wrong option string.
I will add "return -EINVAL" when undefined string is input.
>
>> mutex_lock(&vmpr->events_lock);
>> list_add(&ev->node, &vmpr->events);
> --
> Michal Hocko
> SUSE Labs
Thanks for you comments.
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>
--
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>