[PATCH] vmpressure: implement strict mode

[PATCH] vmpressure: implement strict mode

[Luiz Capitulino]

Currently, applications are notified for the level they registered for
_plus_ higher levels.

This is a problem if the application wants to implement different
actions for different levels. For example, an application might want
to release 10% of its cache on level low, 50% on medium and 100% on
critical. To do this, the application has to register a different fd
for each event. However, fd low is always going to be notified and
and all fds are going to be notified on level critical.

Strict mode solves this problem by strictly notifiying the event
an fd has registered for. It's optional. By default we still notify
on higher levels.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---

PS: I'm following the discussion on the event storm problem, but I believe
    strict mode is orthogonal to what has been suggested (although the
    patches conflict)

 Documentation/cgroups/memory.txt | 10 ++++++----
 mm/vmpressure.c                  | 19 +++++++++++++++++--
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
index ddf4f93..3c589cf 100644
--- a/Documentation/cgroups/memory.txt
+++ b/Documentation/cgroups/memory.txt
@@ -807,12 +807,14 @@ register a notification, an application must:
 
 - create an eventfd using eventfd(2);
 - open memory.pressure_level;
-- write string like "<event_fd> <fd of memory.pressure_level> <level>"
+- write string like "<event_fd> <fd of memory.pressure_level> <level> [strict]"
   to cgroup.event_control.
 
-Application will be notified through eventfd when memory pressure is at
-the specific level (or higher). Read/write operations to
-memory.pressure_level are no implemented.
+Applications will be notified through eventfd when memory pressure is at
+the specific level or higher. If strict is passed, then applications
+will only be notified when memory pressure reaches the specified level.
+
+Read/write operations to memory.pressure_level are no implemented.
 
 Test:
 
diff --git a/mm/vmpressure.c b/mm/vmpressure.c
index 736a601..6289ede 100644
--- a/mm/vmpressure.c
+++ b/mm/vmpressure.c
@@ -137,6 +137,7 @@ static enum vmpressure_levels vmpressure_calc_level(unsigned long scanned,
 struct vmpressure_event {
 	struct eventfd_ctx *efd;
 	enum vmpressure_levels level;
+	bool strict_mode;
 	struct list_head node;
 };
 
@@ -153,6 +154,9 @@ static bool vmpressure_event(struct vmpressure *vmpr,
 
 	list_for_each_entry(ev, &vmpr->events, node) {
 		if (level >= ev->level) {
+			/* strict mode ensures level == ev->level */
+			if (ev->strict_mode && level != ev->level)
+				continue;
 			eventfd_signal(ev->efd, 1);
 			signalled = true;
 		}
@@ -292,7 +296,7 @@ void vmpressure_prio(gfp_t gfp, struct mem_cgroup *memcg, int prio)
  * infrastructure, so that the notifications will be delivered to the
  * @eventfd. The @args parameter is a string that denotes pressure level
  * threshold (one of vmpressure_str_levels, i.e. "low", "medium", or
- * "critical").
+ * "critical") and optionally a different operating mode (i.e. "strict")
  *
  * 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 +307,33 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
 {
 	struct vmpressure *vmpr = cg_to_vmpressure(cg);
 	struct vmpressure_event *ev;
+	bool smode = false;
+	const char *p;
 	int level;
 
 	for (level = 0; level < VMPRESSURE_NUM_LEVELS; level++) {
-		if (!strcmp(vmpressure_str_levels[level], args))
+		p = vmpressure_str_levels[level];
+		if (!strncmp(p, args, strlen(p)))
 			break;
 	}
 
 	if (level >= VMPRESSURE_NUM_LEVELS)
 		return -EINVAL;
 
+	p = strchr(args, ' ');
+	if (p) {
+		if (strncmp(++p, "strict", 6))
+			return -EINVAL;
+		smode = true;
+	}
+
 	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
 	if (!ev)
 		return -ENOMEM;
 
 	ev->efd = eventfd;
 	ev->level = level;
+	ev->strict_mode = smode;
 
 	mutex_lock(&vmpr->events_lock);
 	list_add(&ev->node, &vmpr->events);
-- 
1.8.1.4

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Kyungmin Park]

+ Ms. Kim,

she already raised this issue at another mail thread.

Thank you,
Kyungmin Park

On Wed, Jun 26, 2013 at 6:51 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> Currently, applications are notified for the level they registered for
> _plus_ higher levels.
>
> This is a problem if the application wants to implement different
> actions for different levels. For example, an application might want
> to release 10% of its cache on level low, 50% on medium and 100% on
> critical. To do this, the application has to register a different fd
> for each event. However, fd low is always going to be notified and
> and all fds are going to be notified on level critical.
>
> Strict mode solves this problem by strictly notifiying the event
> an fd has registered for. It's optional. By default we still notify
> on higher levels.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>
> PS: I'm following the discussion on the event storm problem, but I believe
>     strict mode is orthogonal to what has been suggested (although the
>     patches conflict)
>
>  Documentation/cgroups/memory.txt | 10 ++++++----
>  mm/vmpressure.c                  | 19 +++++++++++++++++--
>  2 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> index ddf4f93..3c589cf 100644
> --- a/Documentation/cgroups/memory.txt
> +++ b/Documentation/cgroups/memory.txt
> @@ -807,12 +807,14 @@ register a notification, an application must:
>
>  - create an eventfd using eventfd(2);
>  - open memory.pressure_level;
> -- write string like "<event_fd> <fd of memory.pressure_level> <level>"
> +- write string like "<event_fd> <fd of memory.pressure_level> <level> [strict]"
>    to cgroup.event_control.
>
> -Application will be notified through eventfd when memory pressure is at
> -the specific level (or higher). Read/write operations to
> -memory.pressure_level are no implemented.
> +Applications will be notified through eventfd when memory pressure is at
> +the specific level or higher. If strict is passed, then applications
> +will only be notified when memory pressure reaches the specified level.
> +
> +Read/write operations to memory.pressure_level are no implemented.
>
>  Test:
>
> diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> index 736a601..6289ede 100644
> --- a/mm/vmpressure.c
> +++ b/mm/vmpressure.c
> @@ -137,6 +137,7 @@ static enum vmpressure_levels vmpressure_calc_level(unsigned long scanned,
>  struct vmpressure_event {
>         struct eventfd_ctx *efd;
>         enum vmpressure_levels level;
> +       bool strict_mode;
>         struct list_head node;
>  };
>
> @@ -153,6 +154,9 @@ static bool vmpressure_event(struct vmpressure *vmpr,
>
>         list_for_each_entry(ev, &vmpr->events, node) {
>                 if (level >= ev->level) {
> +                       /* strict mode ensures level == ev->level */
> +                       if (ev->strict_mode && level != ev->level)
> +                               continue;
>                         eventfd_signal(ev->efd, 1);
>                         signalled = true;
>                 }
> @@ -292,7 +296,7 @@ void vmpressure_prio(gfp_t gfp, struct mem_cgroup *memcg, int prio)
>   * infrastructure, so that the notifications will be delivered to the
>   * @eventfd. The @args parameter is a string that denotes pressure level
>   * threshold (one of vmpressure_str_levels, i.e. "low", "medium", or
> - * "critical").
> + * "critical") and optionally a different operating mode (i.e. "strict")
>   *
>   * 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 +307,33 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
>  {
>         struct vmpressure *vmpr = cg_to_vmpressure(cg);
>         struct vmpressure_event *ev;
> +       bool smode = false;
> +       const char *p;
>         int level;
>
>         for (level = 0; level < VMPRESSURE_NUM_LEVELS; level++) {
> -               if (!strcmp(vmpressure_str_levels[level], args))
> +               p = vmpressure_str_levels[level];
> +               if (!strncmp(p, args, strlen(p)))
>                         break;
>         }
>
>         if (level >= VMPRESSURE_NUM_LEVELS)
>                 return -EINVAL;
>
> +       p = strchr(args, ' ');
> +       if (p) {
> +               if (strncmp(++p, "strict", 6))
> +                       return -EINVAL;
> +               smode = true;
> +       }
> +
>         ev = kzalloc(sizeof(*ev), GFP_KERNEL);
>         if (!ev)
>                 return -ENOMEM;
>
>         ev->efd = eventfd;
>         ev->level = level;
> +       ev->strict_mode = smode;
>
>         mutex_lock(&vmpr->events_lock);
>         list_add(&ev->node, &vmpr->events);
> --
> 1.8.1.4
>
> --
> 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>

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Hyunhee Kim]

Please see "[PATCH v3] memcg: event control at vmpressure". mail
thread. (and also the thread I sent last Saturday.)
There was discussion on this mode not sending lower events when "level
!= ev->level".

Thanks,
Hyunhee Kim.

2013/6/26 Kyungmin Park <kmpark@infradead.org>:
> + Ms. Kim,
>
> she already raised this issue at another mail thread.
>
> Thank you,
> Kyungmin Park
>
> On Wed, Jun 26, 2013 at 6:51 AM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> Currently, applications are notified for the level they registered for
>> _plus_ higher levels.
>>
>> This is a problem if the application wants to implement different
>> actions for different levels. For example, an application might want
>> to release 10% of its cache on level low, 50% on medium and 100% on
>> critical. To do this, the application has to register a different fd
>> for each event. However, fd low is always going to be notified and
>> and all fds are going to be notified on level critical.
>>
>> Strict mode solves this problem by strictly notifiying the event
>> an fd has registered for. It's optional. By default we still notify
>> on higher levels.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
>>
>> PS: I'm following the discussion on the event storm problem, but I believe
>>     strict mode is orthogonal to what has been suggested (although the
>>     patches conflict)
>>
>>  Documentation/cgroups/memory.txt | 10 ++++++----
>>  mm/vmpressure.c                  | 19 +++++++++++++++++--
>>  2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
>> index ddf4f93..3c589cf 100644
>> --- a/Documentation/cgroups/memory.txt
>> +++ b/Documentation/cgroups/memory.txt
>> @@ -807,12 +807,14 @@ register a notification, an application must:
>>
>>  - create an eventfd using eventfd(2);
>>  - open memory.pressure_level;
>> -- write string like "<event_fd> <fd of memory.pressure_level> <level>"
>> +- write string like "<event_fd> <fd of memory.pressure_level> <level> [strict]"
>>    to cgroup.event_control.
>>
>> -Application will be notified through eventfd when memory pressure is at
>> -the specific level (or higher). Read/write operations to
>> -memory.pressure_level are no implemented.
>> +Applications will be notified through eventfd when memory pressure is at
>> +the specific level or higher. If strict is passed, then applications
>> +will only be notified when memory pressure reaches the specified level.
>> +
>> +Read/write operations to memory.pressure_level are no implemented.
>>
>>  Test:
>>
>> diff --git a/mm/vmpressure.c b/mm/vmpressure.c
>> index 736a601..6289ede 100644
>> --- a/mm/vmpressure.c
>> +++ b/mm/vmpressure.c
>> @@ -137,6 +137,7 @@ static enum vmpressure_levels vmpressure_calc_level(unsigned long scanned,
>>  struct vmpressure_event {
>>         struct eventfd_ctx *efd;
>>         enum vmpressure_levels level;
>> +       bool strict_mode;
>>         struct list_head node;
>>  };
>>
>> @@ -153,6 +154,9 @@ static bool vmpressure_event(struct vmpressure *vmpr,
>>
>>         list_for_each_entry(ev, &vmpr->events, node) {
>>                 if (level >= ev->level) {
>> +                       /* strict mode ensures level == ev->level */
>> +                       if (ev->strict_mode && level != ev->level)
>> +                               continue;
>>                         eventfd_signal(ev->efd, 1);
>>                         signalled = true;
>>                 }
>> @@ -292,7 +296,7 @@ void vmpressure_prio(gfp_t gfp, struct mem_cgroup *memcg, int prio)
>>   * infrastructure, so that the notifications will be delivered to the
>>   * @eventfd. The @args parameter is a string that denotes pressure level
>>   * threshold (one of vmpressure_str_levels, i.e. "low", "medium", or
>> - * "critical").
>> + * "critical") and optionally a different operating mode (i.e. "strict")
>>   *
>>   * 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 +307,33 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
>>  {
>>         struct vmpressure *vmpr = cg_to_vmpressure(cg);
>>         struct vmpressure_event *ev;
>> +       bool smode = false;
>> +       const char *p;
>>         int level;
>>
>>         for (level = 0; level < VMPRESSURE_NUM_LEVELS; level++) {
>> -               if (!strcmp(vmpressure_str_levels[level], args))
>> +               p = vmpressure_str_levels[level];
>> +               if (!strncmp(p, args, strlen(p)))
>>                         break;
>>         }
>>
>>         if (level >= VMPRESSURE_NUM_LEVELS)
>>                 return -EINVAL;
>>
>> +       p = strchr(args, ' ');
>> +       if (p) {
>> +               if (strncmp(++p, "strict", 6))
>> +                       return -EINVAL;
>> +               smode = true;
>> +       }
>> +
>>         ev = kzalloc(sizeof(*ev), GFP_KERNEL);
>>         if (!ev)
>>                 return -ENOMEM;
>>
>>         ev->efd = eventfd;
>>         ev->level = level;
>> +       ev->strict_mode = smode;
>>
>>         mutex_lock(&vmpr->events_lock);
>>         list_add(&ev->node, &vmpr->events);
>> --
>> 1.8.1.4
>>
>> --
>> 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>
>
> --
> 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>

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Luiz Capitulino]

On Wed, 26 Jun 2013 10:12:15 +0900
Hyunhee Kim <hyunhee.kim@samsung.com> wrote:

> Please see "[PATCH v3] memcg: event control at vmpressure". mail
> thread. (and also the thread I sent last Saturday.)
> There was discussion on this mode not sending lower events when "level
> != ev->level".

The new argument this patch adds should be orthogonal to what has been
discussed and suggested in that thread. The patches conflict though, but
it's just a matter of rebasing if both are ACKed.

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Anton Vorontsov]

On Tue, Jun 25, 2013 at 05:51:29PM -0400, Luiz Capitulino wrote:
> Currently, applications are notified for the level they registered for
> _plus_ higher levels.
> 
> This is a problem if the application wants to implement different
> actions for different levels. For example, an application might want
> to release 10% of its cache on level low, 50% on medium and 100% on
> critical. To do this, the application has to register a different fd
> for each event. However, fd low is always going to be notified and
> and all fds are going to be notified on level critical.
> 
> Strict mode solves this problem by strictly notifiying the event
> an fd has registered for. It's optional. By default we still notify
> on higher levels.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>

In the documentation I would add more information about why exactly the
strict mode makes sense.

For example, the non-strict fd listener hooked onto the low level makes
sense for apps that just monitor reclaiming activity (like current Android
Activity Manager), hooking onto 'medium' non-strict mode makes sense for
simple load-balancing logic, and the new strict mode is for the cases when
an application wants to implement some fancy logic as it makes a decision
based on a concrete level.

Otherwise, it looks good.

Acked-by: Anton Vorontsov <anton@enomsg.org>

Thanks!

Anton

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Luiz Capitulino]

On Tue, 25 Jun 2013 21:03:31 -0700
Anton Vorontsov <anton@enomsg.org> wrote:

> On Tue, Jun 25, 2013 at 05:51:29PM -0400, Luiz Capitulino wrote:
> > Currently, applications are notified for the level they registered for
> > _plus_ higher levels.
> > 
> > This is a problem if the application wants to implement different
> > actions for different levels. For example, an application might want
> > to release 10% of its cache on level low, 50% on medium and 100% on
> > critical. To do this, the application has to register a different fd
> > for each event. However, fd low is always going to be notified and
> > and all fds are going to be notified on level critical.
> > 
> > Strict mode solves this problem by strictly notifiying the event
> > an fd has registered for. It's optional. By default we still notify
> > on higher levels.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> 
> In the documentation I would add more information about why exactly the
> strict mode makes sense.
> 
> For example, the non-strict fd listener hooked onto the low level makes
> sense for apps that just monitor reclaiming activity (like current Android
> Activity Manager), hooking onto 'medium' non-strict mode makes sense for
> simple load-balancing logic, and the new strict mode is for the cases when
> an application wants to implement some fancy logic as it makes a decision
> based on a concrete level.

OK, I'll respin. But you said it all already, so I'll base my text on
on what you wrote.

> Otherwise, it looks good.
> 
> Acked-by: Anton Vorontsov <anton@enomsg.org>

Thanks.

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Minchan Kim]

On Tue, Jun 25, 2013 at 05:51:29PM -0400, Luiz Capitulino wrote:
> Currently, applications are notified for the level they registered for
> _plus_ higher levels.
> 
> This is a problem if the application wants to implement different
> actions for different levels. For example, an application might want
> to release 10% of its cache on level low, 50% on medium and 100% on
> critical. To do this, the application has to register a different fd
> for each event. However, fd low is always going to be notified and
> and all fds are going to be notified on level critical.
> 
> Strict mode solves this problem by strictly notifiying the event
> an fd has registered for. It's optional. By default we still notify
> on higher levels.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Acked-by: Minchan Kim <minchan@kernel.org>

Shouldn't we make this default?
What do you think about it?

-- 
Kind regards,
Minchan Kim

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Michal Hocko]

On Wed 26-06-13 16:50:51, Minchan Kim wrote:
> On Tue, Jun 25, 2013 at 05:51:29PM -0400, Luiz Capitulino wrote:
> > Currently, applications are notified for the level they registered for
> > _plus_ higher levels.
> > 
> > This is a problem if the application wants to implement different
> > actions for different levels. For example, an application might want
> > to release 10% of its cache on level low, 50% on medium and 100% on
> > critical. To do this, the application has to register a different fd
> > for each event. However, fd low is always going to be notified and
> > and all fds are going to be notified on level critical.
> > 
> > Strict mode solves this problem by strictly notifiying the event
> > an fd has registered for. It's optional. By default we still notify
> > on higher levels.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Acked-by: Minchan Kim <minchan@kernel.org>
> 
> Shouldn't we make this default?

The interface is not there for long but still, changing it is always
quite tricky. And the users who care can be modified really easily so I
would stick with the original default.

-- 
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>

Re: [PATCH] vmpressure: implement strict mode

[Minchan Kim]

Hello Michal,

On Wed, Jun 26, 2013 at 09:59:21AM +0200, Michal Hocko wrote:
> On Wed 26-06-13 16:50:51, Minchan Kim wrote:
> > On Tue, Jun 25, 2013 at 05:51:29PM -0400, Luiz Capitulino wrote:
> > > Currently, applications are notified for the level they registered for
> > > _plus_ higher levels.
> > > 
> > > This is a problem if the application wants to implement different
> > > actions for different levels. For example, an application might want
> > > to release 10% of its cache on level low, 50% on medium and 100% on
> > > critical. To do this, the application has to register a different fd
> > > for each event. However, fd low is always going to be notified and
> > > and all fds are going to be notified on level critical.
> > > 
> > > Strict mode solves this problem by strictly notifiying the event
> > > an fd has registered for. It's optional. By default we still notify
> > > on higher levels.
> > > 
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > Acked-by: Minchan Kim <minchan@kernel.org>
> > 
> > Shouldn't we make this default?
> 
> The interface is not there for long but still, changing it is always
> quite tricky. And the users who care can be modified really easily so I
> would stick with the original default.

Yeb, I am not strong against to stick old at a moment but at least,
this patch makes more sense to me so I'd like to know why we didn't do it
from the beginning. Surely, Anton has a answer.

> 
> -- 
> 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>

-- 
Kind regards,
Minchan Kim

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Luiz Capitulino]

On Wed, 26 Jun 2013 17:20:40 +0900
Minchan Kim <minchan@kernel.org> wrote:

> Hello Michal,
> 
> On Wed, Jun 26, 2013 at 09:59:21AM +0200, Michal Hocko wrote:
> > On Wed 26-06-13 16:50:51, Minchan Kim wrote:
> > > On Tue, Jun 25, 2013 at 05:51:29PM -0400, Luiz Capitulino wrote:
> > > > Currently, applications are notified for the level they registered for
> > > > _plus_ higher levels.
> > > > 
> > > > This is a problem if the application wants to implement different
> > > > actions for different levels. For example, an application might want
> > > > to release 10% of its cache on level low, 50% on medium and 100% on
> > > > critical. To do this, the application has to register a different fd
> > > > for each event. However, fd low is always going to be notified and
> > > > and all fds are going to be notified on level critical.
> > > > 
> > > > Strict mode solves this problem by strictly notifiying the event
> > > > an fd has registered for. It's optional. By default we still notify
> > > > on higher levels.
> > > > 
> > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > Acked-by: Minchan Kim <minchan@kernel.org>
> > > 
> > > Shouldn't we make this default?
> > 
> > The interface is not there for long but still, changing it is always
> > quite tricky. And the users who care can be modified really easily so I
> > would stick with the original default.
> 
> Yeb, I am not strong against to stick old at a moment but at least,
> this patch makes more sense to me so I'd like to know why we didn't do it
> from the beginning. Surely, Anton has a answer.

That's exactly my thinking too: I think strict mode should be the default
mode, and the current mode should be optional. But it's not a big deal.

I've discussed this issue with Anton some weeks ago, and iirc (Anton,
please correct/clarify where appropriate) the conclusion was that the
current schema makes sense for apps monitoring reclaim activity, as
they can hook on low only.

Hmm. Something just crossed my mind. Maybe we should have two
notification schemas:

 o memory.pressure_level: implements strict mode (this patch)

 o memory.reclaim_activity: apps are notified whenever there's reclaim
							activity

As for changing applications, it's better to get some breakage while
we're in -rc than regret the API later.

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Anton Vorontsov]

On Wed, Jun 26, 2013 at 04:50:51PM +0900, Minchan Kim wrote:
> On Tue, Jun 25, 2013 at 05:51:29PM -0400, Luiz Capitulino wrote:
> > Currently, applications are notified for the level they registered for
> > _plus_ higher levels.
> > 
> > This is a problem if the application wants to implement different
> > actions for different levels. For example, an application might want
> > to release 10% of its cache on level low, 50% on medium and 100% on
> > critical. To do this, the application has to register a different fd
> > for each event. However, fd low is always going to be notified and
> > and all fds are going to be notified on level critical.
> > 
> > Strict mode solves this problem by strictly notifiying the event
> > an fd has registered for. It's optional. By default we still notify
> > on higher levels.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Acked-by: Minchan Kim <minchan@kernel.org>
> 
> Shouldn't we make this default?
> What do you think about it?

Either way works and both modes have their use-cases (as I have described
in my previous mail). Changing the default mode is just unneeded churn,
IMO. As long as things are documented properly, we are good. :)

Thanks,

Anton

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Michal Hocko]

On Tue 25-06-13 17:51:29, Luiz Capitulino wrote:
> Currently, applications are notified for the level they registered for
> _plus_ higher levels.
> 
> This is a problem if the application wants to implement different
> actions for different levels. For example, an application might want
> to release 10% of its cache on level low, 50% on medium and 100% on
> critical. To do this, the application has to register a different fd
> for each event. However, fd low is always going to be notified and
> and all fds are going to be notified on level critical.

OK, I am not user of this interface but I thought that the application
would take an action of the highest level it gets notification. But yes
this might get clumsy to implement.

> Strict mode solves this problem by strictly notifiying the event
> an fd has registered for. It's optional. By default we still notify
> on higher levels.

OK, makes some sense to me and it should work with the proposed edge
trigerring as well.

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> 
> PS: I'm following the discussion on the event storm problem, but I believe
>     strict mode is orthogonal to what has been suggested (although the
>     patches conflict)
> 
>  Documentation/cgroups/memory.txt | 10 ++++++----
>  mm/vmpressure.c                  | 19 +++++++++++++++++--
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> index ddf4f93..3c589cf 100644
> --- a/Documentation/cgroups/memory.txt
> +++ b/Documentation/cgroups/memory.txt
> @@ -807,12 +807,14 @@ register a notification, an application must:
>  
>  - create an eventfd using eventfd(2);
>  - open memory.pressure_level;
> -- write string like "<event_fd> <fd of memory.pressure_level> <level>"
> +- write string like "<event_fd> <fd of memory.pressure_level> <level> [strict]"
>    to cgroup.event_control.
>  
> -Application will be notified through eventfd when memory pressure is at
> -the specific level (or higher). Read/write operations to
> -memory.pressure_level are no implemented.
> +Applications will be notified through eventfd when memory pressure is at
> +the specific level or higher. If strict is passed, then applications
> +will only be notified when memory pressure reaches the specified level.

It would be good to describe when is strick and when the default
appropriate.

> +
> +Read/write operations to memory.pressure_level are no implemented.
>  
>  Test:
>  
> diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> index 736a601..6289ede 100644
> --- a/mm/vmpressure.c
> +++ b/mm/vmpressure.c
> @@ -137,6 +137,7 @@ static enum vmpressure_levels vmpressure_calc_level(unsigned long scanned,
>  struct vmpressure_event {
>  	struct eventfd_ctx *efd;
>  	enum vmpressure_levels level;
> +	bool strict_mode;

Any reason to not using a flag for this? If there are any other modes to
come them we would end up with zilions of bools which is not very nice.

>  	struct list_head node;
>  };
>  
> @@ -153,6 +154,9 @@ static bool vmpressure_event(struct vmpressure *vmpr,
>  
>  	list_for_each_entry(ev, &vmpr->events, node) {
>  		if (level >= ev->level) {
> +			/* strict mode ensures level == ev->level */
> +			if (ev->strict_mode && level != ev->level)
> +				continue;
>  			eventfd_signal(ev->efd, 1);
>  			signalled = true;
>  		}
> @@ -292,7 +296,7 @@ void vmpressure_prio(gfp_t gfp, struct mem_cgroup *memcg, int prio)
>   * infrastructure, so that the notifications will be delivered to the
>   * @eventfd. The @args parameter is a string that denotes pressure level
>   * threshold (one of vmpressure_str_levels, i.e. "low", "medium", or
> - * "critical").
> + * "critical") and optionally a different operating mode (i.e. "strict")
>   *
>   * 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 +307,33 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
>  {
>  	struct vmpressure *vmpr = cg_to_vmpressure(cg);
>  	struct vmpressure_event *ev;
> +	bool smode = false;
> +	const char *p;
>  	int level;
>  
>  	for (level = 0; level < VMPRESSURE_NUM_LEVELS; level++) {
> -		if (!strcmp(vmpressure_str_levels[level], args))
> +		p = vmpressure_str_levels[level];
> +		if (!strncmp(p, args, strlen(p)))
>  			break;
>  	}
>  
>  	if (level >= VMPRESSURE_NUM_LEVELS)
>  		return -EINVAL;
>  
> +	p = strchr(args, ' ');
> +	if (p) {
> +		if (strncmp(++p, "strict", 6))
> +			return -EINVAL;
> +		smode = true;
> +	}
> +
>  	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
>  	if (!ev)
>  		return -ENOMEM;
>  
>  	ev->efd = eventfd;
>  	ev->level = level;
> +	ev->strict_mode = smode;
>  
>  	mutex_lock(&vmpr->events_lock);
>  	list_add(&ev->node, &vmpr->events);
> -- 
> 1.8.1.4
> 
> --
> 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>

-- 
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>

Re: [PATCH] vmpressure: implement strict mode

[Luiz Capitulino]

On Wed, 26 Jun 2013 10:08:27 +0200
Michal Hocko <mhocko@suse.cz> wrote:

> On Tue 25-06-13 17:51:29, Luiz Capitulino wrote:
> > Currently, applications are notified for the level they registered for
> > _plus_ higher levels.
> > 
> > This is a problem if the application wants to implement different
> > actions for different levels. For example, an application might want
> > to release 10% of its cache on level low, 50% on medium and 100% on
> > critical. To do this, the application has to register a different fd
> > for each event. However, fd low is always going to be notified and
> > and all fds are going to be notified on level critical.
> 
> OK, I am not user of this interface but I thought that the application
> would take an action of the highest level it gets notification. But yes
> this might get clumsy to implement.
> 
> > Strict mode solves this problem by strictly notifiying the event
> > an fd has registered for. It's optional. By default we still notify
> > on higher levels.
> 
> OK, makes some sense to me and it should work with the proposed edge
> trigerring as well.
> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> > 
> > PS: I'm following the discussion on the event storm problem, but I believe
> >     strict mode is orthogonal to what has been suggested (although the
> >     patches conflict)
> > 
> >  Documentation/cgroups/memory.txt | 10 ++++++----
> >  mm/vmpressure.c                  | 19 +++++++++++++++++--
> >  2 files changed, 23 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> > index ddf4f93..3c589cf 100644
> > --- a/Documentation/cgroups/memory.txt
> > +++ b/Documentation/cgroups/memory.txt
> > @@ -807,12 +807,14 @@ register a notification, an application must:
> >  
> >  - create an eventfd using eventfd(2);
> >  - open memory.pressure_level;
> > -- write string like "<event_fd> <fd of memory.pressure_level> <level>"
> > +- write string like "<event_fd> <fd of memory.pressure_level> <level> [strict]"
> >    to cgroup.event_control.
> >  
> > -Application will be notified through eventfd when memory pressure is at
> > -the specific level (or higher). Read/write operations to
> > -memory.pressure_level are no implemented.
> > +Applications will be notified through eventfd when memory pressure is at
> > +the specific level or higher. If strict is passed, then applications
> > +will only be notified when memory pressure reaches the specified level.
> 
> It would be good to describe when is strick and when the default
> appropriate.

Yeah, Anton asked for the same thing. Will do it.

> > +
> > +Read/write operations to memory.pressure_level are no implemented.
> >  
> >  Test:
> >  
> > diff --git a/mm/vmpressure.c b/mm/vmpressure.c
> > index 736a601..6289ede 100644
> > --- a/mm/vmpressure.c
> > +++ b/mm/vmpressure.c
> > @@ -137,6 +137,7 @@ static enum vmpressure_levels vmpressure_calc_level(unsigned long scanned,
> >  struct vmpressure_event {
> >  	struct eventfd_ctx *efd;
> >  	enum vmpressure_levels level;
> > +	bool strict_mode;
> 
> Any reason to not using a flag for this? If there are any other modes to
> come them we would end up with zilions of bools which is not very nice.

Good point, I'll change it.

> 
> >  	struct list_head node;
> >  };
> >  
> > @@ -153,6 +154,9 @@ static bool vmpressure_event(struct vmpressure *vmpr,
> >  
> >  	list_for_each_entry(ev, &vmpr->events, node) {
> >  		if (level >= ev->level) {
> > +			/* strict mode ensures level == ev->level */
> > +			if (ev->strict_mode && level != ev->level)
> > +				continue;
> >  			eventfd_signal(ev->efd, 1);
> >  			signalled = true;
> >  		}
> > @@ -292,7 +296,7 @@ void vmpressure_prio(gfp_t gfp, struct mem_cgroup *memcg, int prio)
> >   * infrastructure, so that the notifications will be delivered to the
> >   * @eventfd. The @args parameter is a string that denotes pressure level
> >   * threshold (one of vmpressure_str_levels, i.e. "low", "medium", or
> > - * "critical").
> > + * "critical") and optionally a different operating mode (i.e. "strict")
> >   *
> >   * 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 +307,33 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
> >  {
> >  	struct vmpressure *vmpr = cg_to_vmpressure(cg);
> >  	struct vmpressure_event *ev;
> > +	bool smode = false;
> > +	const char *p;
> >  	int level;
> >  
> >  	for (level = 0; level < VMPRESSURE_NUM_LEVELS; level++) {
> > -		if (!strcmp(vmpressure_str_levels[level], args))
> > +		p = vmpressure_str_levels[level];
> > +		if (!strncmp(p, args, strlen(p)))
> >  			break;
> >  	}
> >  
> >  	if (level >= VMPRESSURE_NUM_LEVELS)
> >  		return -EINVAL;
> >  
> > +	p = strchr(args, ' ');
> > +	if (p) {
> > +		if (strncmp(++p, "strict", 6))
> > +			return -EINVAL;
> > +		smode = true;
> > +	}
> > +
> >  	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
> >  	if (!ev)
> >  		return -ENOMEM;
> >  
> >  	ev->efd = eventfd;
> >  	ev->level = level;
> > +	ev->strict_mode = smode;
> >  
> >  	mutex_lock(&vmpr->events_lock);
> >  	list_add(&ev->node, &vmpr->events);
> > -- 
> > 1.8.1.4
> > 
> > --
> > 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>
> 

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Pavel Machek]

Hi!

> diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> index ddf4f93..3c589cf 100644
> --- a/Documentation/cgroups/memory.txt
> +++ b/Documentation/cgroups/memory.txt
> @@ -807,12 +807,14 @@ register a notification, an application must:
>  
>  - create an eventfd using eventfd(2);
>  - open memory.pressure_level;
> -- write string like "<event_fd> <fd of memory.pressure_level> <level>"
> +- write string like "<event_fd> <fd of memory.pressure_level> <level> [strict]"
>    to cgroup.event_control.
>  

This is.. pretty strange interface. Would it be cleaner to do ioctl()?
New syscall?

> @@ -303,22 +307,33 @@ int vmpressure_register_event(struct cgroup *cg, struct cftype *cft,
>  {
>  	struct vmpressure *vmpr = cg_to_vmpressure(cg);
>  	struct vmpressure_event *ev;
> +	bool smode = false;
> +	const char *p;
>  	int level;
>  
>  	for (level = 0; level < VMPRESSURE_NUM_LEVELS; level++) {
> -		if (!strcmp(vmpressure_str_levels[level], args))
> +		p = vmpressure_str_levels[level];
> +		if (!strncmp(p, args, strlen(p)))
>  			break;
>  	}
>  
>  	if (level >= VMPRESSURE_NUM_LEVELS)
>  		return -EINVAL;
>  
> +	p = strchr(args, ' ');
> +	if (p) {
> +		if (strncmp(++p, "strict", 6))
> +			return -EINVAL;
> +		smode = true;
> +	}
> +

This looks like something for bash, not for kernel :-(.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Luiz Capitulino]

On Mon, 1 Jul 2013 10:51:03 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> Hi!
> 
> > diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> > index ddf4f93..3c589cf 100644
> > --- a/Documentation/cgroups/memory.txt
> > +++ b/Documentation/cgroups/memory.txt
> > @@ -807,12 +807,14 @@ register a notification, an application must:
> >  
> >  - create an eventfd using eventfd(2);
> >  - open memory.pressure_level;
> > -- write string like "<event_fd> <fd of memory.pressure_level> <level>"
> > +- write string like "<event_fd> <fd of memory.pressure_level> <level> [strict]"
> >    to cgroup.event_control.
> >  
> 
> This is.. pretty strange interface. Would it be cleaner to do ioctl()?
> New syscall?

Are you referring to my new mode or to the whole thing?

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Pavel Machek]

On Tue 2013-07-02 11:06:28, Luiz Capitulino wrote:
> On Mon, 1 Jul 2013 10:51:03 +0200
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > Hi!
> > 
> > > diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> > > index ddf4f93..3c589cf 100644
> > > --- a/Documentation/cgroups/memory.txt
> > > +++ b/Documentation/cgroups/memory.txt
> > > @@ -807,12 +807,14 @@ register a notification, an application must:
> > >  
> > >  - create an eventfd using eventfd(2);
> > >  - open memory.pressure_level;
> > > -- write string like "<event_fd> <fd of memory.pressure_level> <level>"
> > > +- write string like "<event_fd> <fd of memory.pressure_level> <level> [strict]"
> > >    to cgroup.event_control.
> > >  
> > 
> > This is.. pretty strange interface. Would it be cleaner to do ioctl()?
> > New syscall?
> 
> Are you referring to my new mode or to the whole thing?

Well. The interface was already very strange and you made it even
worse.

For example... How is the string terminated? \0? \n? Not at all? Needs
to be written in single write? How many spaces are allowed? Can I put
there \t? How many leading zeros? How do you know that you can stop
looking for "strict" string?

Dunno. Passing fd by writting it out in ascii is strange. (And writing
fd in ascii to the same fd is ... very strange.)

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

--
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>

Re: [PATCH] vmpressure: implement strict mode

[Luiz Capitulino]

On Tue, 2 Jul 2013 21:47:03 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> On Tue 2013-07-02 11:06:28, Luiz Capitulino wrote:
> > On Mon, 1 Jul 2013 10:51:03 +0200
> > Pavel Machek <pavel@ucw.cz> wrote:
> > 
> > > Hi!
> > > 
> > > > diff --git a/Documentation/cgroups/memory.txt b/Documentation/cgroups/memory.txt
> > > > index ddf4f93..3c589cf 100644
> > > > --- a/Documentation/cgroups/memory.txt
> > > > +++ b/Documentation/cgroups/memory.txt
> > > > @@ -807,12 +807,14 @@ register a notification, an application must:
> > > >  
> > > >  - create an eventfd using eventfd(2);
> > > >  - open memory.pressure_level;
> > > > -- write string like "<event_fd> <fd of memory.pressure_level> <level>"
> > > > +- write string like "<event_fd> <fd of memory.pressure_level> <level> [strict]"
> > > >    to cgroup.event_control.
> > > >  
> > > 
> > > This is.. pretty strange interface. Would it be cleaner to do ioctl()?
> > > New syscall?
> > 
> > Are you referring to my new mode or to the whole thing?
> 
> Well. The interface was already very strange and you made it even
> worse.

The existing interface is the cgroup's notification mechanism, I think
discussing it is a bit out of scope for my extension.

Now, regarding my extension itself and the current vmpressure API, I
believe that delivering all events to user-space (ie. w/o any filtering
in the kernel) is a better solution.

Point is whether we can do it with the current vmpressure API (which
is cgroup based) or whether we should move to something else.

--
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>