On Wed, 20 Oct 2021, Michal Hocko wrote:
> On Tue 19-10-21 15:32:27, Neil Brown wrote:
[clip looks of discussion where we are largely in agreement - happy to
see that!]
> > Presumably there is a real risk of deadlock if we just remove the
> > memory-reserves boosts of __GFP_NOFAIL. Maybe it would be safe to
> > replace all current users of __GFP_NOFAIL with __GFP_NOFAIL|__GFP_HIGH,
> > and then remove the __GFP_HIGH where analysis suggests there is no risk
> > of deadlocks.
>
> I would much rather not bind those together and go other way around. If
> somebody can actually hit deadlocks (those are quite easy to spot as
> they do not go away) then we can talk about how to deal with them.
> Memory reserves can help only > < this much.
I recall maybe 10 years ago Linus saying that he preferred simplicity to
mathematical provability for handling memory deadlocks (or something
like that). I lean towards provability myself, but I do see the other
perspective.
We have mempools and they can provide strong guarantees (though they are
often over-allocated I think). But they can be a bit clumsy. I believe
that DaveM is strong against anything like that in the network layer, so
we strongly depend on GFP_MEMALLOC functionality for swap-over-NFS. I'm
sure it is important elsewhere too.
Of course __GFP_HIGH and __GFP_ATOMIC provide an intermediate priority
level - more likely to fail than __GFP_MEMALLOC. I suspect they should
not be seen as avoiding deadlock, only as improving service. So using
them when we cannot wait might make sense, but there are probably other
circumstances.
> > Why is __GFP_NOFAIL better?
>
> Because the allocator can do something if it knows that the allocation
> cannot fail. E.g. give such an allocation a higher priority over those
> that are allowed to fail. This is not limited to memory reserves,
> although this is the only measure that is implemented currently IIRC.
> On the other hand if there is something interesting the caller can do
> directly - e.g. do internal object management like mempool does - then
> it is better to retry at that level.
It *can* do something, but I don't think it *should* do something - not
if that could have a negative impact on other threads. Just because I
cannot fail, that doesn't mean someone else should fail to help me.
Maybe I should just wait longer.
>
> > > * Using this flag for costly allocations is _highly_ discouraged.
> >
> > This is unhelpful. Saying something is "discouraged" carries an implied
> > threat. This is open source and threats need to be open.
> > Why is it discouraged? IF it is not forbidden, then it is clearly
> > permitted. Maybe there are costs - so a clear statement of those costs
> > would be appropriate.
> > Also, what is a suitable alternative?
> >
> > Current code will trigger a WARN_ON, so it is effectively forbidden.
> > Maybe we should document that __GFP_NOFAIL is forbidden for orders above
> > 1, and that vmalloc() should be used instead (thanks for proposing that
> > patch!).
>
> I think we want to recommend kvmalloc as an alternative once vmalloc is
> NOFAIL aware.
>
> I will skip over some of the specific regarding SLAB and NOFS usage if
> you do not mind and focus on points that have direct documentation
> consequences. Also I do not feel qualified commenting on neither SLAB
> nor FS internals.
>
> [...]
> > There is a lot of stuff there.... the bits that are important to me are:
> >
> > - why is __GFP_NOFAIL preferred? It is a valuable convenience, but I
> > don't see that it is necessary
>
> I think it is preferred for one and a half reasons. It tells allocator
> that this allocation cannot really fail and the caller doesn't have a
> very good/clever retry policy (e.g. like mempools mentioned above). The
> half reason would be for tracking purposes (git grep __GFP_NOFAIL) is
> easier than trying to catch all sorts of while loops over allocation
> which do not do anything really interesting.
I think the one reason is misguided, as described above.
I think the half reason is good, and that we should introduce
memalloc_retry_wait()
and encourage developers to use that for any memalloc retry loop.
__GFP_NOFAIL would then be a convenience flag which causes the allocator
(slab or alloc_page or whatever) to call memalloc_retry_wait() and do
the loop internally.
What exactly memalloc_retry_wait() does (if anything) can be decided
separately and changed as needed.
> > - is it reasonable to use __GFP_HIGH when looping if there is a risk of
> > deadlock?
>
> As I've said above. Memory reserves are a finite resource and as such
> they cannot fundamentally solve deadlocks. They can help prioritize
> though.
To be fair, they can solve 1 level of deadlock. i.e. if you only need
to make one allocation to guarantee progress, then allocating from
reserves can help. If you might need to make a second allocation
without freeing the first - then a single reserve pool can provide
guarantees (which is why we use mempool is layered block devices - md
over dm over loop of scsi).
>
> > - Will __GFP_DIRECT_RECLAIM always result in a delay before failure? In
> > that case it should be safe to loop around allocations using
> > __GFP_DIRECT_RECLAIM without needing congestion_wait() (so it can
> > just be removed.
>
> This is a good question and I do not think we have that documented
> anywhere. We do cond_resched() for sure. I do not think we guarantee a
> sleeping point in general. Maybe we should, I am not really sure.
If we add memalloc_retry_wait(), it wouldn't matter. We would only need
to ensure that memalloc_retry_wait() waited if page_alloc didn't.
I think we should:
- introduce memalloc_retry_wait() and use it for all malloc retry loops
including __GFP_NOFAIL
- drop all the priority boosts added for __GFP_NOFAIL
- drop __GFP_ATOMIC and change all the code that tests for __GFP_ATOMIC
to instead test for __GFP_HIGH. __GFP_ATOMIC is NEVER used without
__GFP_HIGH. This give a slight boost to several sites that use
__GFP_HIGH explicitly.
- choose a consistent order threshold for disallowing __GFP_NOFAIL
(rmqueue uses "order > 1", __alloc_pages_slowpath uses
"order > PAGE_ALLOC_COSTLY_ORDER"), test it once - early - and
document kvmalloc as an alternative. Code can also loop if there
is an alternative strategy for freeing up memory.
Thanks,
NeilBrown
Thanks,
NeilBrown
