Re: [PATCH] mm: vmscan: add tracepoints for node reclaim

[Vlastimil Babka <vbabka@suse.cz>]

On 2/28/19 11:34 AM, Yafang Shao wrote:
> On Thu, Feb 28, 2019 at 6:21 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>>
>> On 2/28/19 9:14 AM, Yafang Shao wrote:
>>> In the page alloc fast path, it may do node reclaim, which may cause
>>> latency spike.
>>> We should add tracepoint for this event, and also mesure the latency
>>> it causes.
>>>
>>> So bellow two tracepoints are introduced,
>>>       mm_vmscan_node_reclaim_begin
>>>       mm_vmscan_node_reclaim_end
>>>
>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>> ---
>>>  include/trace/events/vmscan.h | 48 +++++++++++++++++++++++++++++++++++++++++++
>>>  mm/vmscan.c                   | 13 +++++++++++-
>>>  2 files changed, 60 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
>>> index a1cb913..9310d5b 100644
>>> --- a/include/trace/events/vmscan.h
>>> +++ b/include/trace/events/vmscan.h
>>> @@ -465,6 +465,54 @@
>>>               __entry->ratio,
>>>               show_reclaim_flags(__entry->reclaim_flags))
>>>  );
>>> +
>>> +TRACE_EVENT(mm_vmscan_node_reclaim_begin,
>>> +
>>> +     TP_PROTO(int nid, int order, int may_writepage,
>>> +             gfp_t gfp_flags, int zid),
>>> +
>>> +     TP_ARGS(nid, order, may_writepage, gfp_flags, zid),
>>> +
>>> +     TP_STRUCT__entry(
>>> +             __field(int, nid)
>>> +             __field(int, order)
>>> +             __field(int, may_writepage)
>>
>> For node reclaim may_writepage is statically set in node_reclaim_mode,
>> so I'm not sure it's worth including it.
>>
>>> +             __field(gfp_t, gfp_flags)
>>> +             __field(int, zid)
>>
>> zid seems wasteful and misleading as it's simply derived by
>> gfp_zone(gfp_mask), so I would drop it.
>>
> 
> I agree with you that may_writepage and zid is wasteful, but I found
> they are in other tracepoints in this file,
> so I place them in this tracepoint as well.

I see zid only in kswapd waking tracepoints? That's different kind of
event.

> Seems we'd better drop them from other tracepoints as well ?

Hmm seems may_writepage in other tracepoints depends on laptop_mode
which is also a static setting. do_try_to_free_pages() can override it
due to priority, but that doesn't affect the tracepoints. If they are to
be dropped, it would be a separate patch though.