vmevent: question?

DISCUSSION Minchan Kim 3 participants 11 messages Apr 30, 2012 – May 3, 2012 Memory management

lore

Share & tools

lore

Minchan Kim

author

2012-04-30 07:06 UTC #0 lore raw

Hi Pekka,

I looked into vmevent and have few questions.

vmevent_smaple gathers all registered values to report to user if vmevent match. But the time gap between vmevent match check and vmevent_sample_attr could make error so user could confuse.

Q 1. Why do we report _all_ registered vmstat value?
In my opinion, it's okay just to report _a_ value vmevent_match happens.
Q 2. Is it okay although value when vmevent_match check happens is different with
vmevent_sample_attr in vmevent_sample's for loop?
I think it's not good.
Q 3. Do you have any plan to change getting value's method?
Now it's IRQ context so we have limitation to get a vmstat values so that It couldn't be generic. IMHO, To merge into mainline, we should solve this problem.
Q 4. Do you have any plan for this patchset to merge into mainline?

Thanks.

Signature · 9 lines

Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

2012-04-30 07:35 UTC #1 lore raw

Hi Minchan,

Quoted 6 lines · Minchan Kim

vmevent_smaple gathers all registered values to report to user if vmevent match. But the time gap between vmevent match check and vmevent_sample_attr could make error so user could confuse.
Q 1. Why do we report _all_ registered vmstat value?
In my opinion, it's okay just to report _a_ value vmevent_match happens.

It makes the userspace side simpler for "lowmem notification" use case. I'm open to changing the ABI if it doesn't make the userspace side too complex.

Quoted 3 lines

Q 2. Is it okay although value when vmevent_match check happens is different with vmevent_sample_attr in vmevent_sample's for loop?
I think it's not good.

Yeah, that's just silly and needs fixing.

Quoted 3 lines

Q 3. Do you have any plan to change getting value's method?
Now it's IRQ context so we have limitation to get a vmstat values so that It couldn't be generic. IMHO, To merge into mainline, we should solve this problem.

Yes, that needs fixing as well. I was hoping to reuse perf sampling code for this.

Quoted 1 line

Q 4. Do you have any plan for this patchset to merge into mainline?

Yes, I'm interested in pushing it forward if we can show that the ABI makes sense, is stable and generic enough, and fixes real world problems.

Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Minchan Kim

author

2012-04-30 07:52 UTC #2 lore raw

Quoted 7 lines · Pekka Enberg

Hi Minchan,
Quoted 4 lines · Minchan Kim
vmevent_smaple gathers all registered values to report to user if vmevent match. But the time gap between vmevent match check and vmevent_sample_attr could make error so user could confuse.

Quoted 6 lines

Quoted 2 lines
Q 1. Why do we report _all_ registered vmstat value?
In my opinion, it's okay just to report _a_ value vmevent_match happens.
It makes the userspace side simpler for "lowmem notification" use case. I'm open to changing the ABI if it doesn't make the userspace side too complex.

Yes. I understand your point but if we still consider all of values, we don't have any way to capture exact values except triggered event value. I mean there is no lock to keep consistency.
If stale data is okay, no problem but IMHO, it could make user very confusing. So let's return value for first matched event if various event match. Of course, let's write down it in ABI.
If there is other idea for reporting all of item with consistent, I'm okay.

Quoted 5 lines

Quoted 3 lines
Q 2. Is it okay although value when vmevent_match check happens is different with
vmevent_sample_attr in vmevent_sample's for loop?
I think it's not good.
Yeah, that's just silly and needs fixing.

It depends on Q.1. So first of all, we have to determine Q 1's policy.

Quoted 5 lines

Quoted 4 lines
Q 3. Do you have any plan to change getting value's method?
Now it's IRQ context so we have limitation to get a vmstat values so that It couldn't be generic. IMHO, To merge into mainline, we should solve this problem.

Quoted 8 lines

Yes, that needs fixing as well. I was hoping to reuse perf sampling code for this.
Quoted 1 line
Q 4. Do you have any plan for this patchset to merge into mainline?
Yes, I'm interested in pushing it forward if we can show that the ABI makes sense, is stable and generic enough, and fixes real world problems.

Yes. I think it would be a good for embedded and KVM world for preventing unnecessary swapped-out and OOM. :)

Signature · 9 lines

Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

2012-04-30 08:01 UTC #3 lore raw

Hi Minchan,

Quoted 11 lines · Minchan Kim

Quoted 3 lines
It makes the userspace side simpler for "lowmem notification" use case. I'm open to changing the ABI if it doesn't make the userspace side too complex.
Yes. I understand your point but if we still consider all of values, we don't have any way to capture exact values except triggered event value. I mean there is no lock to keep consistency.
If stale data is okay, no problem but IMHO, it could make user very confusing. So let's return value for first matched event if various event match. Of course, let's write down it in ABI.
If there is other idea for reporting all of item with consistent, I'm okay.

What kind of consistency guarantees do you mean? The data sent to userspace is always a snapshot of the state and therefore can be stale by the time it reaches userspace.

If your code needs stricter consistency guarantees, you probably want to do it in the kernel.

Pekka

Minchan Kim

author

2012-04-30 08:36 UTC #4 lore raw

Quoted 15 lines · Pekka Enberg

Hi Minchan,
Quoted 11 lines · Minchan Kim
Quoted 3 lines
It makes the userspace side simpler for "lowmem notification" use case. I'm open to changing the ABI if it doesn't make the userspace side too complex.
Yes. I understand your point but if we still consider all of values, we don't have any way to capture exact values except triggered event value. I mean there is no lock to keep consistency.
If stale data is okay, no problem but IMHO, it could make user very confusing. So let's return value for first matched event if various event match. Of course, let's write down it in ABI.
If there is other idea for reporting all of item with consistent, I'm okay.

Quoted 3 lines

What kind of consistency guarantees do you mean? The data sent to userspace is always a snapshot of the state and therefore can be stale by the time it reaches userspace.

Consistency between component of snapshot.
let's assume following as

1. User expect some events's value would be minus when event he expect happen. A : -3, B : -4, C : -5, D : -6
2. Logically, it's not possible to mix plus and minus values for the events. A : -3, B : -4, C : -5, D : -6 ( O )
A : -3, B : -4, C : 1, D : 2 ( X )

But in current implementation, some of those could be minus and some of those could be plus. Which event could user believe?
At least, we need a _captured_ value when event triggered so that user can ignore other values.

Quoted 13 lines

If your code needs stricter consistency guarantees, you probably want to do it in the kernel.
Pekka
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Signature · 9 lines

Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

2012-05-03 07:24 UTC #5 lore raw

Quoted 13 lines · Minchan Kim

Quoted 3 lines
What kind of consistency guarantees do you mean? The data sent to userspace is always a snapshot of the state and therefore can be stale by the time it reaches userspace.
Consistency between component of snapshot.
let's assume following as
1. User expect some events's value would be minus when event he expect happen. A : -3, B : -4, C : -5, D : -6
2. Logically, it's not possible to mix plus and minus values for the events. A : -3, B : -4, C : -5, D : -6 ( O )
A : -3, B : -4, C : 1, D : 2 ( X )

Quoted 3 lines

But in current implementation, some of those could be minus and some of those could be plus. Which event could user believe?
At least, we need a _captured_ value when event triggered so that user can ignore other values.

Sorry, I still don't quite understand the problem.

The current implementation provides the same kind of snapshot consistency as reading from /proc/vmstat does (modulo the fact that we read them twice) for the values we support.

Pekka

Minchan Kim

author

2012-05-03 07:57 UTC #6 lore raw

Quoted 14 lines · Pekka Enberg

Quoted 13 lines · Minchan Kim
Quoted 3 lines
What kind of consistency guarantees do you mean? The data sent to userspace is always a snapshot of the state and therefore can be stale by the time it reaches userspace.
Consistency between component of snapshot.
let's assume following as
1. User expect some events's value would be minus when event he expect happen. A : -3, B : -4, C : -5, D : -6
2. Logically, it's not possible to mix plus and minus values for the events. A : -3, B : -4, C : -5, D : -6 ( O )
A : -3, B : -4, C : 1, D : 2 ( X )

Quoted 5 lines

Quoted 3 lines
But in current implementation, some of those could be minus and some of those could be plus. Which event could user believe?
At least, we need a _captured_ value when event triggered so that user can ignore other values.
Sorry, I still don't quite understand the problem.

Sorry for my poor explanation.
My point is when userspace get vmevent_event by reading fd, it could enumerate
several attribute all at once.
Then, one of attribute(call A) made by vmevent_match in kernel and other attributes(call B, C, D) are just extra for convenience. Because there is time gap when kernel get attribute values, B,C,D could be stale. Then, how can user determine which event is really triggered? A or B or C or D?
Which event really happens?

Quoted 14 lines

The current implementation provides the same kind of snapshot consistency as reading from /proc/vmstat does (modulo the fact that we read them twice) for the values we support.
Pekka
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Signature · 9 lines

Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

2012-05-03 08:07 UTC #7 lore raw

Quoted 7 lines · Minchan Kim

Sorry for my poor explanation.
My point is when userspace get vmevent_event by reading fd, it could enumerate
several attribute all at once.
Then, one of attribute(call A) made by vmevent_match in kernel and other attributes(call B, C, D) are just extra for convenience. Because there is time gap when kernel get attribute values, B,C,D could be stale. Then, how can user determine which event is really triggered? A or B or C or D?
Which event really happens?

Right. Mark the matching values with something like VMEVENT_ATTR_STATE_CAPTURED should be sufficient?

Minchan Kim

author

2012-05-03 08:14 UTC #8 lore raw

Quoted 11 lines · Pekka Enberg

Quoted 7 lines · Minchan Kim
Sorry for my poor explanation.
My point is when userspace get vmevent_event by reading fd, it could enumerate
several attribute all at once.
Then, one of attribute(call A) made by vmevent_match in kernel and other attributes(call B, C, D) are just extra for convenience. Because there is time gap when kernel get attribute values, B,C,D could be stale. Then, how can user determine which event is really triggered? A or B or C or D?
Which event really happens?
Right. Mark the matching values with something like VMEVENT_ATTR_STATE_CAPTURED should be sufficient?

Seems to be good and we have to notice to user by document "Except VMEVENT_ATTR_STATE_CAPTURED, all attributes's value could be stale. So, don't be deceived. Please ignore if you need"

First of all, let make CAPTURED state could be exact.

-----

Quoted 3 lines

Q 2. Is it okay although value when vmevent_match check happens is different with
vmevent_sample_attr in vmevent_sample's for loop?
I think it's not good.

Yeah, that's just silly and needs fixing.
-----

Quoted 8 lines

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Signature · 2 lines

Kind regards,
Minchan Kim

Anton Vorontsov

2012-04-30 07:55 UTC #9 lore raw

In reply to Pekka Enberg

Hello Pekka,

Quoted 10 lines · Pekka Enberg

Quoted 6 lines
vmevent_smaple gathers all registered values to report to user if vmevent match. But the time gap between vmevent match check and vmevent_sample_attr could make error so user could confuse.
Q 1. Why do we report _all_ registered vmstat value?
A A In my opinion, it's okay just to report _a_ value vmevent_match happens.
It makes the userspace side simpler for "lowmem notification" use case. I'm open to changing the ABI if it doesn't make the userspace side too complex.

Yep. Actually, I'd like to add something like 'file_pages - shmem' attribute, and reporting both (i.e. this new attr and free_pages) values at the same time (even if just one crossed the threshold).

Reporting all the values would help userspace logic (so it won't need to read /proc again).

Quoted 5 lines

Quoted 1 line
Q 4. Do you have any plan for this patchset to merge into mainline?
Yes, I'm interested in pushing it forward if we can show that the ABI makes sense, is stable and generic enough, and fixes real world problems.

It seems to be a pretty nice driver. Speaking of ABI, the only thing I personally dislike is VMEVENT_CONFIG_MAX_ATTRS (i.e. fixed-size array in vmevent_config)... but I guess it's pretty easy to make it variable-sized array... was there any particular reason to make the _MAX thing?

Thanks!

Signature · 9 lines

Anton Vorontsov
Email: [email protected]

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

2012-04-30 08:03 UTC #10 lore raw

Hi Anton,

Quoted 5 lines · Anton Vorontsov

It seems to be a pretty nice driver. Speaking of ABI, the only thing I personally dislike is VMEVENT_CONFIG_MAX_ATTRS (i.e. fixed-size array in vmevent_config)... but I guess it's pretty easy to make it variable-sized array... was there any particular reason to make the _MAX thing?

It made the implementation simpler but the ABI should support variable-sized arrays. We can relax the limitation if necessary.

Pekka