On Wed 10-03-21 21:43:16, Andrew Morton wrote:
> On Wed, 10 Mar 2021 18:13:21 -0800 Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
> > put_page does not correctly handle all calling contexts for hugetlb
> > pages. This was recently discussed in the threads [1] and [2].
> >
> > free_huge_page is the routine called for the final put_page of huegtlb
> > pages. Since at least the beginning of git history, free_huge_page has
> > acquired the hugetlb_lock to move the page to a free list and possibly
> > perform other processing. When this code was originally written, the
> > hugetlb_lock should have been made irq safe.
> >
> > For many years, nobody noticed this situation until lockdep code caught
> > free_huge_page being called from irq context. By this time, another
> > lock (hugetlb subpool) was also taken in the free_huge_page path. In
> > addition, hugetlb cgroup code had been added which could hold
> > hugetlb_lock for a considerable period of time. Because of this, commit
> > c77c0a8ac4c5 ("mm/hugetlb: defer freeing of huge pages if in non-task
> > context") was added to address the issue of free_huge_page being called
> > from irq context. That commit hands off free_huge_page processing to a
> > workqueue if !in_task.
> >
> > The !in_task check handles the case of being called from irq context.
> > However, it does not take into account the case when called with irqs
> > disabled as in [1].
> >
> > To complicate matters, functionality has been added to hugetlb
> > such that free_huge_page may block/sleep in certain situations. The
> > hugetlb_lock is of course dropped before potentially blocking.
> >
> > One way to handle all calling contexts is to have free_huge_page always
> > send pages to the workqueue for processing. This idea was briefly
> > discussed here [3], but has some undesirable side effects.
> >
> > Ideally, the hugetlb_lock should have been irq safe from the beginning
> > and any code added to the free_huge_page path should have taken this
> > into account. However, this has not happened. The code today does have
> > the ability to hand off requests to a workqueue. It does this for calls
> > from irq context. Changing the check in the code from !in_task to
> > in_atomic would handle the situations when called with irqs disabled.
> > However, it does not not handle the case when called with a spinlock
> > held. This is needed because the code could block/sleep.
> >
> > Select PREEMPT_COUNT if HUGETLB_PAGE is enabled so that in_atomic can be
> > used to detect all atomic contexts where sleeping is not possible.
> >
> > [1] https://lore.kernel.org/linux-mm/000000000000f1c03b05bc43aadc@xxxxxxxxxx/
> > [2] https://lore.kernel.org/linux-mm/YEjji9oAwHuZaZEt@xxxxxxxxxxxxxx/
> > [3] https://lore.kernel.org/linux-mm/YDzaAWK41K4gD35V@xxxxxxxxxxxxxx/
> >
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -235,6 +235,7 @@ config HUGETLBFS
> >
> > config HUGETLB_PAGE
> > def_bool HUGETLBFS
> > + select PREEMPT_COUNT
> >
>
> Well this is unfortunate. hugetlb is forcing PREEMPT_COUNT because we
> screwed things up.
Yes this is far from ideal but we have tried to explore other ways all
looking much more complex. [1] shows that this is a problem already and
needs a reasonable fix to be backported for older kernels.
> Did we consider changing the networking code to call a new
> free_huge_tlb_from_irq()? So the callee doesn't need to guess.
I do not think we want to pollute networking or any other code that
simply wants to put_page with a hugetlb specific knowledge.
> Or something else?
>
> Is anyone looking onto fixing this for real?
Mike said he would be looking into making hugetlb_lock irq safe but
there is a non trivial way there and this would be not a great candidate
for backporting.
Btw. RCU already wants to have a reliable in_atomic as well and that
effectivelly means enabling PREEMPT_COUNT for everybody. The overhead of
per-cpu preempt counter should pretty much invisible AFAIK.
--
Michal Hocko
SUSE Labs
