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

On Tue 18-06-13 17:00:06, Hyunhee Kim wrote:

2013/6/18 Hyunhee Kim [off-list ref]:

quoted

2013/6/17 Michal Hocko [off-list ref]:

quoted

On Mon 17-06-13 20:30:11, Hyunhee Kim wrote:
[...]

quoted

diff --git a/mm/vmpressure.c b/mm/vmpressure.c
index 736a601..a18fdb3 100644
--- a/mm/vmpressure.c
+++ b/mm/vmpressure.c

[...]

quoted

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

quoted

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

Is there any guarantee that such a condition would be also signalled?

quoted

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.

I think the interface should be consistent with `always' unless there is
very good reason to do otherwise.

quoted

Currently, as you mentioned, for edge_trigger option, I'll remove
"level != ev->level" part.

quoted

quoted

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

How about initializing vmpr->last_level = -1 everytime new event is
registered? (having last_level per vmpr). 

So all those consumers that have seen an event already would be
surprised that they get the very same event again without transition to
other level (so it won't be edge triggered anymore). No this doesn't
make any sense to me.

Please try to think about the interface, what it is supposed to do and
how it is supposed to behave. The current implementation seems hackish
to me and it is an example of a single-use-case-designed interface which
tend to be hard to maintain and a bad idea in long term.

I think that if we have last_level for each event, only new event
could be triggered when the current level is same as the last
level. And I think that this is a little awkward.

Why? I might be wrong here but when I register an event I would like to
get a notification when the event is triggered for the first time from
my POV. I have no way to find out that such an event has been already
triggered for somebody else.

But, if we set vmpr->last_level = -1 when new event is registered,
we can see all events with new event even though the level is not
changed.

Which basically ruins the idea of the edge triggered event.

[...]
-- 
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>