On Fri 08-01-21 17:01:03, Muchun Song wrote: > On Fri, Jan 8, 2021 at 4:43 PM Michal Hocko <mho...@suse.com> wrote: > > > > On Thu 07-01-21 23:11:22, Muchun Song wrote: [..] > > > But I find a tricky problem to solve. See free_huge_page(). > > > If we are in non-task context, we should schedule a work > > > to free the page. We reuse the page->mapping. If the page > > > is already freed by the dissolve path. We should not touch > > > the page->mapping. So we need to check PageHuge(). > > > The check and llist_add() should be protected by > > > hugetlb_lock. But we cannot do that. Right? If dissolve > > > happens after it is linked to the list. We also should > > > remove it from the list (hpage_freelist). It seems to make > > > the thing more complex. > > > > I am not sure I follow you here but yes PageHuge under hugetlb_lock > > should be the reliable way to check for the race. I am not sure why we > > really need to care about mapping or other state. > > CPU0: CPU1: > free_huge_page(page) > if (PageHuge(page)) > dissolve_free_huge_page(page) > spin_lock(&hugetlb_lock) > update_and_free_page(page) > spin_unlock(&hugetlb_lock) > llist_add(page->mapping) > // the mapping is corrupted > > The PageHuge(page) and llist_add() should be protected by > hugetlb_lock. Right? If so, we cannot hold hugetlb_lock > in free_huge_page() path.
OK, I see. I completely forgot about this snowflake. I thought that free_huge_page was a typo missing initial __. Anyway you are right that this path needs a check as well. But I don't see why we couldn't use the lock here. The lock can be held only inside the !in_task branch. Although it would be much more nicer if the lock was held at this layer rather than both free_huge_page and __free_huge_page. But that clean up can be done on top. -- Michal Hocko SUSE Labs
