On Tue, Mar 14, 2023 at 10:01:06PM +0100, Michal Hocko wrote:
> On Tue 14-03-23 15:49:09, Marcelo Tosatti wrote:
> > On Tue, Mar 14, 2023 at 03:31:21PM +0100, Michal Hocko wrote:
> > > On Tue 14-03-23 09:59:37, Marcelo Tosatti wrote:
> > > > On Tue, Mar 14, 2023 at 01:25:53PM +0100, Michal Hocko wrote:
> > > > > On Mon 13-03-23 13:25:07, Marcelo Tosatti wrote:
> > > > > > This patch series addresses the following two problems:
> > > > > >
> > > > > > 1. A customer provided some evidence which indicates that
> > > > > > the idle tick was stopped; albeit, CPU-specific vmstat
> > > > > > counters still remained populated.
> > > > > >
> > > > > > Thus one can only assume quiet_vmstat() was not
> > > > > > invoked on return to the idle loop. If I understand
> > > > > > correctly, I suspect this divergence might erroneously
> > > > > > prevent a reclaim attempt by kswapd. If the number of
> > > > > > zone specific free pages are below their per-cpu drift
> > > > > > value then zone_page_state_snapshot() is used to
> > > > > > compute a more accurate view of the aforementioned
> > > > > > statistic. Thus any task blocked on the NUMA node
> > > > > > specific pfmemalloc_wait queue will be unable to make
> > > > > > significant progress via direct reclaim unless it is
> > > > > > killed after being woken up by kswapd
> > > > > > (see throttle_direct_reclaim())
> > > > >
> > > > > I have hard time to follow the actual problem described above. Are you
> > > > > suggesting that a lack of pcp vmstat counters update has led to
> > > > > reclaim issues? What is the said "evidence"? Could you share more of the
> > > > > story please?
> > > >
> > > >
> > > > - The process was trapped in throttle_direct_reclaim().
> > > > The function wait_event_killable() was called to wait condition
> > > > allow_direct_reclaim(pgdat) for current node to be true.
> > > > The allow_direct_reclaim(pgdat) examined the number of free pages
> > > > on the node by zone_page_state() which just returns value in
> > > > zone->vm_stat[NR_FREE_PAGES].
> > > >
> > > > - On node #1, zone->vm_stat[NR_FREE_PAGES] was 0.
> > > > However, the freelist on this node was not empty.
> > > >
> > > > - This inconsistent of vmstat value was caused by percpu vmstat on
> > > > nohz_full cpus. Every increment/decrement of vmstat is performed
> > > > on percpu vmstat counter at first, then pooled diffs are cumulated
> > > > to the zone's vmstat counter in timely manner. However, on nohz_full
> > > > cpus (in case of this customer's system, 48 of 52 cpus) these pooled
> > > > diffs were not cumulated once the cpu had no event on it so that
> > > > the cpu started sleeping infinitely.
> > > > I checked percpu vmstat and found there were total 69 counts not
> > > > cumulated to the zone's vmstat counter yet.
> > > >
> > > > - In this situation, kswapd did not help the trapped process.
> > > > In pgdat_balanced(), zone_wakermark_ok_safe() examined the number
> > > > of free pages on the node by zone_page_state_snapshot() which
> > > > checks pending counts on percpu vmstat.
> > > > Therefore kswapd could know there were 69 free pages correctly.
> > > > Since zone->_watermark = {8, 20, 32}, kswapd did not work because
> > > > 69 was greater than 32 as high watermark.
> > >
> > > If the imprecision of allow_direct_reclaim is the underlying problem why
> > > haven't you used zone_page_state_snapshot instead?
> >
> > It might have dealt with problem #1 for this particular case. However,
> > looking at the callers of zone_page_state:
> >
> > 5 2227 mm/compaction.c <<compaction_suitable>>
> > zone_page_state(zone, NR_FREE_PAGES));
> > 6 124 mm/highmem.c <<__nr_free_highpages>>
> > pages += zone_page_state(zone, NR_FREE_PAGES);
> > 7 283 mm/page-writeback.c <<node_dirtyable_memory>>
> > nr_pages += zone_page_state(zone, NR_FREE_PAGES);
> > 8 318 mm/page-writeback.c <<highmem_dirtyable_memory>>
> > nr_pages = zone_page_state(z, NR_FREE_PAGES);
> > 9 321 mm/page-writeback.c <<highmem_dirtyable_memory>>
> > nr_pages += zone_page_state(z, NR_ZONE_INACTIVE_FILE);
> > 10 322 mm/page-writeback.c <<highmem_dirtyable_memory>>
> > nr_pages += zone_page_state(z, NR_ZONE_ACTIVE_FILE);
> > 11 3091 mm/page_alloc.c <<__rmqueue>>
> > zone_page_state(zone, NR_FREE_CMA_PAGES) >
> > 12 3092 mm/page_alloc.c <<__rmqueue>>
> > zone_page_state(zone, NR_FREE_PAGES) / 2) {
> >
> > The suggested patchset fixes the problem of where due to nohz_full,
> > the delayed timer for vmstat_work can be armed but not executed (which means
> > the per-cpu counters can be out of sync for as long as the cpu is in
> > idle while in nohz_full mode).
> >
> > You probably do not want to convert all callers of zone_page_state
> > into zone_page_state_snapshot (as a justification for the proposed
> > patchset).
>
> Yes, I do not really think we want or even need to convert all of them.
OK.
> But it seems that your direct reclaim throttling example really requires
> that. The thing with the remote flushing is that it would suffer from
> a similar imprecations as the flushing could be deferred and under
> certain conditions really starved.
> So it is definitely worth fixing the
> issue you are seeing without such a complex scheme.
The scheme is necessary for other reasons.
> > > Anyway, this is kind of information that is really helpful to have in
> > > the patch description.
> >
> > Agree: resending a new version with updated commit.
>
> I would really recommend trying out the simple fix and see if it changes
> the behavior.
>
> > > [...]
> > > > > > 2. With a SCHED_FIFO task that busy loops on a given CPU,
> > > > > > and kworker for that CPU at SCHED_OTHER priority,
> > > > > > queuing work to sync per-vmstats will either cause that
> > > > > > work to never execute, or stalld (i.e. stall daemon)
> > > > > > boosts kworker priority which causes a latency
> > > > > > violation
> > > > >
> > > > > Why is that a problem? Out-of-sync stats shouldn't cause major problems.
> > > > > Or can they?
> > > >
> > > > Consider SCHED_FIFO task that is polling the network queue (say
> > > > testpmd).
> > > >
> > > > do {
> > > > if (net_registers->state & DATA_AVAILABLE) {
> > > > process_data)();
> > > > }
> > > > } while (!stopped);
> > > >
> > > > Since this task runs at SCHED_FIFO priority, kworker won't
> > > > be scheduled to run (therefore per-CPU vmstats won't be
> > > > flushed to global vmstats).
> > >
> > > Yes, that is certainly possible. But my main point is that vmstat
> > > imprecision shouldn't cause functional problems. That is why we have
> > > _snapshot readers to get an exact value where it matters for
> > > consistency.
> >
> > Understood. Perhaps allow_direct_reclaim should use
> > zone_page_state_snapshot, as otherwise it is only precise
> > at sysctl_stat_interval intervals?
>
> or even much less than that. The flusher uses WQ infrastructure and even
> when a WQ_MEM_RECLAIM one is used this doesn't mean that all workers
> could be jammed.
>
> > > > Or, if testpmd runs at SCHED_OTHER, then the work item to
> > > > flush per-CPU vmstats causes
> > > >
> > > > testpmd -> kworker
> > > > kworker: flush per-CPU vmstats
> > > > kworker -> testpmd
> > >
> > > And this might cause undesired latencies to the packets being
> > > processed by the testpmd task.
> >
> > > Right but can you have any latencies expectation in a situation like
> > > that?
> >
> > Not sure i understand what you mean. Example:
> >
> > https://www.gabrieleara.it/assets/documents/papers/conferences/2021-ieee-nfv-sdn.pdf
> >
> > In general, UDPDK exhibits a much lower
> > latency than the in-kernel UDP stack used through the POSIX
> > API (e.g., a 69 % reduction from 95 µs down to 29 µs), thanks
> > to its ability to bypass the kernel entirely, which in turn
> > outperforms the in-kernel TCP stack as available through the
> > POSIX API, as expected.
> > ...
> > Alternatively, application processes can use UDPDK
> > with the non-blocking API calls (using the O_NONBLOCK flag)
> > and perform some other action while waiting for packets to
> > be ready to be sent/received to/from the UDPDK Process,
> > instead of performing continuous busy-loops on packet queues.
> > However, in this case the cost of a single CPU fully busy due
> > to the UDPDK Process itself is anyway unavoidab
>
> If the userspace workload avoids the kernel completely then it is quite
> unlikely that there is any pcp work to be flushed for in-kernel
> counters.
This particular workload avoids the kernel. Others (were latency is
still a concern) don't.
> That being said, I am nor saying remote flushing is not useful.
> I just think that the issue you are reporting here could be fixed by
> a much simpler fix that doesn't change the way how the flushing is
> performed.
OK. Must change flushing anyway, but fixing allow_direct_reclaim to
use zone_page_state_snapshot won't hurt.
> Such a large rework should be justified by performance numbers.
OK.
> It should be also explained how do we end up doing a lot of work on
> isolated cpus or a pure user space workload.
Again, pure user space workload is one example where latency matters, in
response to the "can you have any latencies expectation in a situation like
that?" question.
Will resend -v8 with allow_direct_reclaim fix.
