On Thu, Dec 12, 2024 at 6:44 AM Sebastian Andrzej Siewior
<bigeasy@xxxxxxxxxxxxx> wrote:
>
> On 2024-12-10 14:49:14 [-0800], Alexei Starovoitov wrote:
> > On Tue, Dec 10, 2024 at 12:35 AM Sebastian Andrzej Siewior
> > <bigeasy@xxxxxxxxxxxxx> wrote:
> > >
> > > On 2024-12-09 18:39:32 [-0800], Alexei Starovoitov wrote:
> > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > index d511e68903c6..a969a62ec0c3 100644
> > > > --- a/mm/page_alloc.c
> > > > +++ b/mm/page_alloc.c
> > > > @@ -1251,9 +1254,33 @@ static void free_one_page(struct zone *zone, struct page *page,
> > > > unsigned long pfn, unsigned int order,
> > > > fpi_t fpi_flags)
> > > > {
> > > > + struct llist_head *llhead;
> > > > unsigned long flags;
> > > >
> > > > - spin_lock_irqsave(&zone->lock, flags);
> > > > + if (!spin_trylock_irqsave(&zone->lock, flags)) {
> > > > + if (unlikely(fpi_flags & FPI_TRYLOCK)) {
> > > > + /* Remember the order */
> > > > + page->order = order;
> > > > + /* Add the page to the free list */
> > > > + llist_add(&page->pcp_llist, &zone->trylock_free_pages);
> > > > + return;
> > > > + }
> > > > + spin_lock_irqsave(&zone->lock, flags);
> > > > + }
> > > > +
> > > > + /* The lock succeeded. Process deferred pages. */
> > > > + llhead = &zone->trylock_free_pages;
> > > > + if (unlikely(!llist_empty(llhead))) {
> > > > + struct llist_node *llnode;
> > > > + struct page *p, *tmp;
> > > > +
> > > > + llnode = llist_del_all(llhead);
> > >
> > > Do you really need to turn the list around?
> >
> > I didn't think LIFO vs FIFO would make a difference.
> > Why spend time rotating it?
>
> I'm sorry. I read llist_reverse_order() in there but it is not there. So
> it is all good.
>
> > > > + llist_for_each_entry_safe(p, tmp, llnode, pcp_llist) {
> > > > + unsigned int p_order = p->order;
> > > > + split_large_buddy(zone, p, page_to_pfn(p), p_order, fpi_flags);
> > > > + __count_vm_events(PGFREE, 1 << p_order);
> > > > + }
> > >
> > > We had something like that (returning memory in IRQ/ irq-off) in RT tree
> > > and we got rid of it before posting the needed bits to mm.
> > >
> > > If we really intend to do something like this, could we please process
> > > this list in an explicitly locked section? I mean not in a try-lock
> > > fashion which might have originated in an IRQ-off region on PREEMPT_RT
> > > but in an explicit locked section which would remain preemptible. This
> > > would also avoid the locking problem down the road when
> > > shuffle_pick_tail() invokes get_random_u64() which in turn acquires a
> > > spinlock_t.
> >
> > I see. So the concern is though spin_lock_irqsave(&zone->lock)
> > is sleepable in RT, bpf prog might have been called in the context
> > where preemption is disabled and do split_large_buddy() for many
> > pages might take too much time?
> Yes.
>
> > How about kicking irq_work then? The callback is in kthread in RT.
> > We can irq_work_queue() right after llist_add().
> >
> > Or we can process only N pages at a time in this loop and
> > llist_add() leftover back into zone->trylock_free_pages.
>
> It could be simpler to not process the trylock_free_pages list in the
> trylock attempt, only in the lock case which is preemptible.
Make sense. Will change to:
/* The lock succeeded. Process deferred pages. */
llhead = &zone->trylock_free_pages;
- if (unlikely(!llist_empty(llhead))) {
+ if (unlikely(!llist_empty(llhead) && !(fpi_flags & FPI_TRYLOCK))) {
