Re: [PATCH] mm/hugetlb: Defer freeing of huge pages if in non-task context

[Waiman Long]

On 12/16/19 5:40 PM, Mike Kravetz wrote:
> On 12/16/19 10:27 AM, Waiman Long wrote:
>> The following lockdep splat was observed when a certain hugetlbfs test
>> was run:
> <snip>
>> This patch implements the deferred freeing by adding a
>> free_hpage_workfn() work function to do the actual freeing. The
>> free_huge_page() call in a non-task context saves the page to be freed
>> in the hpage_freelist linked list in a lockless manner.
>>
>> The generic workqueue is used to process the work, but a dedicated
>> workqueue can be used instead if it is desirable to have the huge page
>> freed ASAP.
>>
> <snip>
>>  
>> +/*
>> + * As free_huge_page() can be called from a non-task context, we have
>> + * to defer the actual freeing in a workqueue to prevent potential
>> + * hugetlb_lock deadlock.
>> + *
>> + * free_hpage_workfn() locklessly retrieves the linked list of pages to
>> + * be freed and frees them one-by-one. As the page->mapping pointer is
>> + * going to be cleared in __free_huge_page() anyway, it is reused as the
>> + * next pointer of a singly linked list of huge pages to be freed.
>> + */
>> +#define NEXT_PENDING	((struct page *)-1)
>> +static struct page *hpage_freelist;
>> +
>> +static void free_hpage_workfn(struct work_struct *work)
>> +{
>> +	struct page *curr, *next;
>> +	int cnt = 0;
>> +
>> +	do {
>> +		curr = xchg(&hpage_freelist, NULL);
>> +		if (!curr)
>> +			break;
>> +
>> +		while (curr) {
>> +			next = (struct page *)READ_ONCE(curr->mapping);
>> +			if (next == NEXT_PENDING) {
>> +				cpu_relax();
>> +				continue;
>> +			}
>> +			__free_huge_page(curr);
>> +			curr = next;
>> +			cnt++;
>> +		}
>> +	} while (!READ_ONCE(hpage_freelist));
>> +
>> +	if (!cnt)
>> +		return;
>> +	pr_debug("HugeTLB: free_hpage_workfn() frees %d huge page(s)\n", cnt);
>> +}
>> +static DECLARE_WORK(free_hpage_work, free_hpage_workfn);
>> +
>> +void free_huge_page(struct page *page)
>> +{
>> +	/*
>> +	 * Defer freeing if in non-task context to avoid hugetlb_lock deadlock.
>> +	 */
>> +	if (!in_task()) {
>> +		struct page *next;
>> +
>> +		page->mapping = (struct address_space *)NEXT_PENDING;
>> +		next = xchg(&hpage_freelist, page);
>> +		WRITE_ONCE(page->mapping, (struct address_space *)next);
>> +		schedule_work(&free_hpage_work);
>> +		return;
>> +	}
> As Andrew mentioned, the design for the lockless queueing could use more
> explanation.  I had to draw some diagrams before I felt relatively confident
> in the design.
>
>> +
>> +	/*
>> +	 * Racing may prevent some deferred huge pages in hpage_freelist
>> +	 * from being freed. Check here and call schedule_work() if that
>> +	 * is the case.
>> +	 */
>> +	if (unlikely(hpage_freelist && !work_pending(&free_hpage_work)))
>> +		schedule_work(&free_hpage_work);
> Can you describe the race which would leave deferred huge pages on
> hpage_freelist?  I am having a hard time determining how that can happen.
I am being cautious here. It is related how the workqueue works. Whether
a call to schedule_work() has any effect depends on the pending bit in
the workqueue structure. I suppose that it is cleared once the work is
done. So depending on when the bit is cleared, there may be a small
timing window where free_hpage_workfn() is done but the bit has not been
cleared yet. A concurrent softIRQ task may update hpage_freelist and
call schedule_work() without actually queuing it. Perhaps I can check
the return status of schedule_work() and wait for a while there until
the queuing is successfully or the free list is changed. I will need to
look more carefully at the workqueue code to see how big this timing
window is.
> And, if this indeed can happen then I would have to ask what happens if
> a page is 'stuck' and we do not call free_huge_page?  Do we need to take
> that case into account?

As said above, there may be way to reduce the racing window or eliminate
it altogether. I need a bit more time to investigate that. If there is
no way to eliminate the racing window, it is possible that a huge page
may get stuck in the free list for a while.

Cheers,
Longman