On Wed, Jul 18, 2018 at 10:50:32AM +0200, Michal Hocko wrote:
> On Wed 18-07-18 00:55:29, Naoya Horiguchi wrote:
> > On Tue, Jul 17, 2018 at 04:27:43PM +0200, Michal Hocko wrote:
> > > On Tue 17-07-18 14:32:31, Naoya Horiguchi wrote:
> > > > There's a race condition between soft offline and hugetlb_fault which
> > > > causes unexpected process killing and/or hugetlb allocation failure.
> > > >
> > > > The process killing is caused by the following flow:
> > > >
> > > > CPU 0 CPU 1 CPU 2
> > > >
> > > > soft offline
> > > > get_any_page
> > > > // find the hugetlb is free
> > > > mmap a hugetlb file
> > > > page fault
> > > > ...
> > > > hugetlb_fault
> > > > hugetlb_no_page
> > > > alloc_huge_page
> > > > // succeed
> > > > soft_offline_free_page
> > > > // set hwpoison flag
> > > > mmap the hugetlb file
> > > > page fault
> > > > ...
> > > > hugetlb_fault
> > > > hugetlb_no_page
> > > > find_lock_page
> > > > return VM_FAULT_HWPOISON
> > > > mm_fault_error
> > > > do_sigbus
> > > > // kill the process
> > > >
> > > >
> > > > The hugetlb allocation failure comes from the following flow:
> > > >
> > > > CPU 0 CPU 1
> > > >
> > > > mmap a hugetlb file
> > > > // reserve all free page but don't fault-in
> > > > soft offline
> > > > get_any_page
> > > > // find the hugetlb is free
> > > > soft_offline_free_page
> > > > // set hwpoison flag
> > > > dissolve_free_huge_page
> > > > // fail because all free hugepages are reserved
> > > > page fault
> > > > ...
> > > > hugetlb_fault
> > > > hugetlb_no_page
> > > > alloc_huge_page
> > > > ...
> > > > dequeue_huge_page_node_exact
> > > > // ignore hwpoisoned hugepage
> > > > // and finally fail due to no-mem
> > > >
> > > > The root cause of this is that current soft-offline code is written
> > > > based on an assumption that PageHWPoison flag should beset at first to
> > > > avoid accessing the corrupted data. This makes sense for memory_failure()
> > > > or hard offline, but does not for soft offline because soft offline is
> > > > about corrected (not uncorrected) error and is safe from data lost.
> > > > This patch changes soft offline semantics where it sets PageHWPoison flag
> > > > only after containment of the error page completes successfully.
> > >
> > > Could you please expand on the worklow here please? The code is really
> > > hard to grasp. I must be missing something because the thing shouldn't
> > > be really complicated. Either the page is in the free pool and you just
> > > remove it from the allocator (with hugetlb asking for a new hugeltb page
> > > to guaratee reserves) or it is used and you just migrate the content to
> > > a new page (again with the hugetlb reserves consideration). Why should
> > > PageHWPoison flag ordering make any relevance?
> >
> > (Considering soft offlining free hugepage,)
> > PageHWPoison is set at first before this patch, which is racy with
> > hugetlb fault code because it's not protected by hugetlb_lock.
> >
> > Originally this was written in the similar manner as hard-offline, where
> > the race is accepted and a PageHWPoison flag is set as soon as possible.
> > But actually that's found not necessary/correct because soft offline is
> > supposed to be less aggressive and failure is OK.
>
> OK
>
> > So this patch is suggesting to make soft-offline less aggressive by
> > moving SetPageHWPoison into the lock.
>
> I guess I still do not understand why we should even care about the
> ordering of the HWPoison flag setting. Why cannot we simply have the
> following code flow? Or maybe we are doing that already I just do not
> follow the code
>
> soft_offline
> check page_count
> - free - normal page - remove from the allocator
> - hugetlb - allocate a new hugetlb page && remove from the pool
> - used - migrate to a new page && never release the old one
>
> Why do we even need HWPoison flag here? Everything can be completely
> transparent to the application. It shouldn't fail from what I
> understood.
PageHWPoison flag is used to the 'remove from the allocator' part
which is like below:
static inline
struct page *rmqueue(
...
do {
page = NULL;
if (alloc_flags & ALLOC_HARDER) {
page = __rmqueue_smallest(zone, order, MIGRATE_HIGHATOMIC);
if (page)
trace_mm_page_alloc_zone_locked(page, order, migratetype);
}
if (!page)
page = __rmqueue(zone, order, migratetype);
} while (page && check_new_pages(page, order));
check_new_pages() returns true if the page taken from free list has
a hwpoison page so that the allocator iterates another round to get
another page.
There's no function that can be called from outside allocator to remove
a page in allocator. So actual page removal is done at allocation time,
not at error handling time. That's the reason why we need PageHWPoison.
Thanks,
Naoya Horiguchi
> > > Do I get it right that the only difference between the hard and soft
> > > offlining is that hugetlb reserves might break for the former while not
> > > for the latter
> >
> > Correct.
> >
> > > and that the failed migration kills all owners for the
> > > former while not for latter?
> >
> > Hard-offline doesn't cause any page migration because the data is already
> > lost, but yes it can kill the owners.
> > Soft-offline never kills processes even if it fails (due to migration failrue
> > or some other reasons.)
> >
> > I listed below some common points and differences between hard-offline
> > and soft-offline.
> >
> > common points
> > - they are both contained by PageHWPoison flag,
> > - error is injected via simliar interfaces.
> >
> > differences
> > - the data on the page is considered lost in hard offline, but is not
> > in soft offline,
> > - hard offline likely kills the affected processes, but soft offline
> > never kills processes,
> > - soft offline causes page migration, but hard offline does not,
> > - hard offline prioritizes to prevent consumption of broken data with
> > accepting some race, and soft offline prioritizes not to impact
> > userspace with accepting failure.
> >
> > Looks to me that there're more differences rather than commont points.
>
> Thanks for the summary. It certainly helped me
> --
> Michal Hocko
> SUSE Labs
>
