Re: [PATCH v2] mm: fs: invalidate bh_lrus for only cold path

[Minchan Kim]

On Tue, Jun 01, 2021 at 04:15:40PM -0700, Andrew Morton wrote:
> On Tue,  1 Jun 2021 07:54:25 -0700 Minchan Kim <minchan@xxxxxxxxxx> wrote:
> 
> > kernel test robot reported the regression of fio.write_iops[1]
> > with [2].
> > 
> > Since lru_add_drain is called frequently, invalidate bh_lrus
> > there could increase bh_lrus cache miss ratio, which needs
> > more IO in the end.
> > 
> > This patch moves the bh_lrus invalidation from the hot path(
> > e.g., zap_page_range, pagevec_release) to cold path(i.e.,
> > lru_add_drain_all, lru_cache_disable).
> 
> This code is starting to hurt my brain.
> 
> What are the locking/context rules for invalidate_bh_lrus_cpu()? 


> AFAICT it offers no protection against two CPUs concurrently running
> __invalidate_bh_lrus() against the same bh_lru.

The lru_add_drain_per_cpu will run on per-cpu since it's per-cpu work
and invalidate_bh_lrus_cpu will run under bh_lru_lock so I couldn't
imagine that race can happen.

> 
> So when CONFIG_SMP=y, invalidate_bh_lrus_cpu() must always and only be
> run on the cpu which owns the bh_lru.  In which case why does it have
> the `cpu' arg?

I just wanted to express both lru_add_drain_cpu and invalidate_bh_lrus_cpu
in lru_add_and_bh_lrus_drain run in the same cpu but look like a bad idea
since it makes people confused. Let me remove the cpu argument from
invalidate_bh_lrus_cpu.

> 
> Your new lru_add_and_bh_lrus_drain() follows these rules by calling
> invalidate_bh_lrus_cpu() from a per-cpu worker or when CONFIG_SMP=n.
> 
> I think.  It's all as clear as mud and undocumented.  Could you please
> take a look at this?  Comment the locking/context rules thoroughly and
> check that they are being followed?  Not forgetting cpu hotplug...  See if
> there's a way of simplifying/clarifying the code?
> 
> The fact that swap.c has those #ifdef CONFIG_SMPs in there is a hint
> that we're doing something wrong (or poorly) in there.  Perhaps that's
> unavoidable because of all the fancy footwork in __lru_add_drain_all().
> 

Hopefully, this is better.