vmevent: question?

vmevent: question?

[Minchan Kim]

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.

-- 
Kind regards,
Minchan Kim

Re: vmevent: question?

[Pekka Enberg]

Hi Minchan,

On Mon, Apr 30, 2012 at 10:06 AM, Minchan Kim <minchan@kernel.org> wrote:
> 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.

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

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

> 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

Re: vmevent: question?

[Minchan Kim]

On 04/30/2012 04:35 PM, Pekka Enberg wrote:

> Hi Minchan,
> 
> On Mon, Apr 30, 2012 at 10:06 AM, Minchan Kim <minchan@kernel.org> wrote:
>> 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.


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.

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

> 
>> 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.
> 
>> 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. :)


-- 
Kind regards,
Minchan Kim

Re: vmevent: question?

[Pekka Enberg]

Hi Minchan,

On Mon, Apr 30, 2012 at 10:52 AM, Minchan Kim <minchan@kernel.org> wrote:
>> 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

Re: vmevent: question?

[Minchan Kim]

On 04/30/2012 05:01 PM, Pekka Enberg wrote:

> Hi Minchan,
> 
> On Mon, Apr 30, 2012 at 10:52 AM, Minchan Kim <minchan@kernel.org> wrote:
>>> 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.


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.

> 
> 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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

Re: vmevent: question?

[Pekka Enberg]

On Mon, 30 Apr 2012, Minchan Kim wrote:
> > 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.

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

Re: vmevent: question?

[Minchan Kim]

On 05/03/2012 04:24 PM, Pekka Enberg wrote:

> On Mon, 30 Apr 2012, Minchan Kim wrote:
>>> 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.
> 
> 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?


> 
> 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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

Re: vmevent: question?

[Pekka Enberg]

On Thu, May 3, 2012 at 10:57 AM, Minchan Kim <minchan@kernel.org> wrote:
> 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?

Re: vmevent: question?

[Minchan Kim]

On 05/03/2012 05:07 PM, Pekka Enberg wrote:

> On Thu, May 3, 2012 at 10:57 AM, Minchan Kim <minchan@kernel.org> wrote:
>> 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.

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

> 
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

Re: vmevent: question?

[Anton Vorontsov]

Hello Pekka,

On Mon, Apr 30, 2012 at 10:35:02AM +0300, Pekka Enberg wrote:
> > 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.

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

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

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: vmevent: question?

[Pekka Enberg]

Hi Anton,

On Mon, Apr 30, 2012 at 10:54 AM, Anton Vorontsov
<cbouatmailru@gmail.com> wrote:
> 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