Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-19 Thread Mike Kravetz
On 2/19/21 2:14 AM, Oscar Salvador wrote: > On Fri, Feb 19, 2021 at 10:56:42AM +0100, Michal Hocko wrote: >> OK, this should work but I am really wondering whether it wouldn't be >> just simpler to replace the old page by a new one in the free list >> directly. Or is there any reason we have to go

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-19 Thread Michal Hocko
On Fri 19-02-21 12:17:11, Oscar Salvador wrote: > On Fri, Feb 19, 2021 at 11:55:00AM +0100, Michal Hocko wrote: > > It is not the lock that I care about but more about counters. The > > intention was that there is a single place to handle both enqueing and > > dequeing. As not all places require

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-19 Thread Oscar Salvador
On Fri, Feb 19, 2021 at 11:55:00AM +0100, Michal Hocko wrote: > It is not the lock that I care about but more about counters. The > intention was that there is a single place to handle both enqueing and > dequeing. As not all places require counters to be updated. E.g. the > migration which just

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-19 Thread Michal Hocko
On Fri 19-02-21 11:40:30, Oscar Salvador wrote: > On Fri, Feb 19, 2021 at 10:56:42AM +0100, Michal Hocko wrote: > > OK, this should work but I am really wondering whether it wouldn't be > > just simpler to replace the old page by a new one in the free list > > directly. Or is there any reason we

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-19 Thread Oscar Salvador
On Fri, Feb 19, 2021 at 10:56:42AM +0100, Michal Hocko wrote: > OK, this should work but I am really wondering whether it wouldn't be > just simpler to replace the old page by a new one in the free list > directly. Or is there any reason we have to go through the generic > helpers path? I mean

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-19 Thread Oscar Salvador
On Fri, Feb 19, 2021 at 10:56:42AM +0100, Michal Hocko wrote: > OK, this should work but I am really wondering whether it wouldn't be > just simpler to replace the old page by a new one in the free list > directly. Or is there any reason we have to go through the generic > helpers path? I mean

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-19 Thread Michal Hocko
On Fri 19-02-21 10:05:53, Oscar Salvador wrote: > On Thu, Feb 18, 2021 at 02:59:40PM +0100, Michal Hocko wrote: > > > It should be: > > > > > > allocate_a_new_page (new_page's refcount = 1) > > > put_page(new_page); (new_page's refcount = 0) > > > dissolve_old_page > > > : if fail > > >

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-19 Thread Oscar Salvador
On Thu, Feb 18, 2021 at 02:59:40PM +0100, Michal Hocko wrote: > > It should be: > > > > allocate_a_new_page (new_page's refcount = 1) > > put_page(new_page); (new_page's refcount = 0) > > dissolve_old_page > > : if fail > > dissolve_new_page (we can dissolve it as refcount == 0) > > >

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-18 Thread Oscar Salvador
On 2021-02-18 14:59, Michal Hocko wrote: As I've said. Page allocator can cope with NULL nodemask just fine. I have checked the code and now remember the tricky part. It is alloc_gigantic_page which cannot work with NULL nodemask because it relies on for_each_node_mask and that, unlike zonelist

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-18 Thread Michal Hocko
On Thu 18-02-21 14:32:50, Oscar Salvador wrote: > On Thu, Feb 18, 2021 at 01:52:38PM +0100, Michal Hocko wrote: > > > Ok, makes sense. > > > __GFP_THISNODE will not allow fallback to other node's zones. > > > Since we only allow the nid the page belongs to, nodemask should be > > > NULL, right? >

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-18 Thread Oscar Salvador
On Thu, Feb 18, 2021 at 01:52:38PM +0100, Michal Hocko wrote: > > Ok, makes sense. > > __GFP_THISNODE will not allow fallback to other node's zones. > > Since we only allow the nid the page belongs to, nodemask should be > > NULL, right? > > I would have to double check because hugetlb has a

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-18 Thread Michal Hocko
On Thu 18-02-21 11:09:17, Oscar Salvador wrote: > On Wed, Feb 17, 2021 at 04:00:11PM +0100, Michal Hocko wrote: > > Is this really necessary? dissolve_free_huge_page will take care of this > > and the race windown you are covering is really tiny. > > Probably not, I was trying to shrink to race

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-18 Thread Oscar Salvador
On Wed, Feb 17, 2021 at 04:00:11PM +0100, Michal Hocko wrote: > Is this really necessary? dissolve_free_huge_page will take care of this > and the race windown you are covering is really tiny. Probably not, I was trying to shrink to race window as much as possible but the call to

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread Michal Hocko
On Wed 17-02-21 11:08:15, Oscar Salvador wrote: [...] > +static bool alloc_and_dissolve_huge_page(struct hstate *h, struct page *page) > +{ > + gfp_t gfp_mask = htlb_alloc_mask(h); > + nodemask_t *nmask = _states[N_MEMORY]; > + struct page *new_page; > + bool ret = false; > +

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread Oscar Salvador
On Wed, Feb 17, 2021 at 03:08:04PM +0100, David Hildenbrand wrote: > On 17.02.21 14:59, Michal Hocko wrote: > > On Wed 17-02-21 14:53:37, David Hildenbrand wrote: > > > On 17.02.21 14:50, Michal Hocko wrote: > > [...] > > > > Do we have any real life examples? Or does this fall more into, let's >

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread Michal Hocko
On Wed 17-02-21 15:08:04, David Hildenbrand wrote: > On 17.02.21 14:59, Michal Hocko wrote: > > On Wed 17-02-21 14:53:37, David Hildenbrand wrote: > > > On 17.02.21 14:50, Michal Hocko wrote: > > [...] > > > > Do we have any real life examples? Or does this fall more into, let's > > > > optimize

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread David Hildenbrand
On 17.02.21 14:59, Michal Hocko wrote: On Wed 17-02-21 14:53:37, David Hildenbrand wrote: On 17.02.21 14:50, Michal Hocko wrote: [...] Do we have any real life examples? Or does this fall more into, let's optimize an existing implementation category. It's a big TODO item I have on my list

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread Michal Hocko
On Wed 17-02-21 14:53:37, David Hildenbrand wrote: > On 17.02.21 14:50, Michal Hocko wrote: [...] > > Do we have any real life examples? Or does this fall more into, let's > > optimize an existing implementation category. > > > > It's a big TODO item I have on my list and I am happy that Oscar

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread David Hildenbrand
On 17.02.21 14:50, Michal Hocko wrote: On Wed 17-02-21 14:36:47, David Hildenbrand wrote: On 17.02.21 14:30, Michal Hocko wrote: On Wed 17-02-21 11:08:15, Oscar Salvador wrote: Free hugetlb pages are tricky to handle so as to no userspace application notices disruption, we need to replace the

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread Michal Hocko
On Wed 17-02-21 14:36:47, David Hildenbrand wrote: > On 17.02.21 14:30, Michal Hocko wrote: > > On Wed 17-02-21 11:08:15, Oscar Salvador wrote: > > > Free hugetlb pages are tricky to handle so as to no userspace application > > > notices disruption, we need to replace the current free hugepage

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread Oscar Salvador
On Wed, Feb 17, 2021 at 02:30:43PM +0100, Michal Hocko wrote: > On Wed 17-02-21 11:08:15, Oscar Salvador wrote: > I do not think fallback to a different zone is ok. If yes then this > really requires a very good reasoning. alloc_contig_range is an > optimistic allocation interface at best and it

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread David Hildenbrand
On 17.02.21 14:30, Michal Hocko wrote: On Wed 17-02-21 11:08:15, Oscar Salvador wrote: Free hugetlb pages are tricky to handle so as to no userspace application notices disruption, we need to replace the current free hugepage with a new one. In order to do that, a new function called

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread Michal Hocko
On Wed 17-02-21 11:08:15, Oscar Salvador wrote: > Free hugetlb pages are tricky to handle so as to no userspace application > notices disruption, we need to replace the current free hugepage with > a new one. > > In order to do that, a new function called alloc_and_dissolve_huge_page > is

[PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

2021-02-17 Thread Oscar Salvador
Free hugetlb pages are tricky to handle so as to no userspace application notices disruption, we need to replace the current free hugepage with a new one. In order to do that, a new function called alloc_and_dissolve_huge_page is introduced. This function will first try to get a new fresh hugetlb