Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1451026 > unrolled thread

Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks NeilBrown <neilb@suse.com> - 2016-07-27 05:50 +0200
    Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-07-27 20:30 +0200
      Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks NeilBrown <neilb@suse.com> - 2016-07-27 23:40 +0200
        Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-07-28 09:20 +0200
          Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-08-03 15:00 +0200
            Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-08-03 17:50 +0200
              Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-08-04 20:50 +0200

#1451026 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jul 25 2016, Michal Hocko wrote:

> On Sat 23-07-16 10:12:24, NeilBrown wrote:

>> Maybe that is impractical, but having firm rules like that would go a
>> long way to make it possible to actually understand and reason about how
>> MM works.  As it is, there seems to be a tendency to put bandaids over
>> bandaids.
>
> Ohh, I would definitely wish for this to be more clear but as it turned
> out over time there are quite some interdependencies between MM/FS/IO
> layers which make the picture really blur. If there is a brave soul to
> make that more clear without breaking any of that it would be really
> cool ;)

Just need that comprehensive regression-test-suite and off we go....


>> > My thinking was that throttle_vm_writeout is there to prevent from
>> > dirtying too many pages from the reclaim the context.  PF_LESS_THROTTLE
>> > is part of the writeout so throttling it on too many dirty pages is
>> > questionable (well we get some bias but that is not really reliable). It
>> > still makes sense to throttle when the backing device is congested
>> > because the writeout path wouldn't make much progress anyway and we also
>> > do not want to cycle through LRU lists too quickly in that case.
>> 
>> "dirtying ... from the reclaim context" ??? What does that mean?
>
> Say you would cause a swapout from the reclaim context. You would
> effectively dirty that anon page until it gets written down to the
> storage.

I should probably figure out how swap really works.  I have vague ideas
which are probably missing important details...
Isn't the first step that the page gets moved into the swap-cache - and
marked dirty I guess.  Then it gets written out and the page is marked
'clean'.
Then further memory pressure might push it out of the cache, or an early
re-use would pull it back from the cache.
If so, then "dirtying in reclaim context" could also be described as
"moving into the swap cache" - yes?  So should there be a limit on dirty
pages in the swap cache just like there is for dirty pages in any
filesystem (the max_dirty_ratio thing) ??
Maybe there is?

>> The use of PF_LESS_THROTTLE in current_may_throttle() in vmscan.c is to
>> avoid a live-lock.  A key premise is that nfsd only allocates unbounded
>> memory when it is writing to the page cache.  So it only needs to be
>> throttled when the backing device it is writing to is congested.  It is
>> particularly important that it *doesn't* get throttled just because an
>> NFS backing device is congested, because nfsd might be trying to clear
>> that congestion.
>
> Thanks for the clarification. IIUC then removing throttle_vm_writeout
> for the nfsd writeout should be harmless as well, right?

Certainly shouldn't hurt from the perspective of nfsd.

>> >> The purpose of that flag is to allow a thread to dirty a page-cache page
>> >> as part of cleaning another page-cache page.
>> >> So it makes sense for loop and sometimes for nfsd.  It would make sense
>> >> for dm-crypt if it was putting the encrypted version in the page cache.
>> >> But if dm-crypt is just allocating a transient page (which I think it
>> >> is), then a mempool should be sufficient (and we should make sure it is
>> >> sufficient) and access to an extra 10% (or whatever) of the page cache
>> >> isn't justified.
>> >
>> > If you think that PF_LESS_THROTTLE (ab)use in mempool_alloc is not
>> > appropriate then would a PF_MEMPOOL be any better?
>> 
>> Why a PF rather than a GFP flag?
>
> Well, short answer is that gfp masks are almost depleted.

Really?  We have 26.

pagemap has a cute hack to store both GFP flags and other flag bits in
the one 32 it number per address_space.  'struct address_space' could
afford an extra 32 number I think.

radix_tree_root adds 3 'tag' flags to the gfp_mask.
There is 16bits of free space in radix_tree_node (between 'offset' and
'count').  That space on the root node could store a record of which tags
are set anywhere.  Or would that extra memory de-ref be a killer?

I think we'd end up with cleaner code if we removed the cute-hacks.  And
we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
need all those 26).

Thanks,
NeilBrown

[toc] | [next] | [standalone]

#1451455 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

On Wed 27-07-16 13:43:35, NeilBrown wrote:
> On Mon, Jul 25 2016, Michal Hocko wrote:
> 
> > On Sat 23-07-16 10:12:24, NeilBrown wrote:
[...]
> >> > My thinking was that throttle_vm_writeout is there to prevent from
> >> > dirtying too many pages from the reclaim the context.  PF_LESS_THROTTLE
> >> > is part of the writeout so throttling it on too many dirty pages is
> >> > questionable (well we get some bias but that is not really reliable). It
> >> > still makes sense to throttle when the backing device is congested
> >> > because the writeout path wouldn't make much progress anyway and we also
> >> > do not want to cycle through LRU lists too quickly in that case.
> >> 
> >> "dirtying ... from the reclaim context" ??? What does that mean?
> >
> > Say you would cause a swapout from the reclaim context. You would
> > effectively dirty that anon page until it gets written down to the
> > storage.
> 
> I should probably figure out how swap really works.  I have vague ideas
> which are probably missing important details...
> Isn't the first step that the page gets moved into the swap-cache - and
> marked dirty I guess.  Then it gets written out and the page is marked
> 'clean'.
> Then further memory pressure might push it out of the cache, or an early
> re-use would pull it back from the cache.
> If so, then "dirtying in reclaim context" could also be described as
> "moving into the swap cache" - yes?

Yes that is basically correct

> So should there be a limit on dirty
> pages in the swap cache just like there is for dirty pages in any
> filesystem (the max_dirty_ratio thing) ??
> Maybe there is?

There is no limit AFAIK. We are relying that the reclaim is throttled
when necessary.
 
> >> The use of PF_LESS_THROTTLE in current_may_throttle() in vmscan.c is to
> >> avoid a live-lock.  A key premise is that nfsd only allocates unbounded
> >> memory when it is writing to the page cache.  So it only needs to be
> >> throttled when the backing device it is writing to is congested.  It is
> >> particularly important that it *doesn't* get throttled just because an
> >> NFS backing device is congested, because nfsd might be trying to clear
> >> that congestion.
> >
> > Thanks for the clarification. IIUC then removing throttle_vm_writeout
> > for the nfsd writeout should be harmless as well, right?
> 
> Certainly shouldn't hurt from the perspective of nfsd.
> 
> >> >> The purpose of that flag is to allow a thread to dirty a page-cache page
> >> >> as part of cleaning another page-cache page.
> >> >> So it makes sense for loop and sometimes for nfsd.  It would make sense
> >> >> for dm-crypt if it was putting the encrypted version in the page cache.
> >> >> But if dm-crypt is just allocating a transient page (which I think it
> >> >> is), then a mempool should be sufficient (and we should make sure it is
> >> >> sufficient) and access to an extra 10% (or whatever) of the page cache
> >> >> isn't justified.
> >> >
> >> > If you think that PF_LESS_THROTTLE (ab)use in mempool_alloc is not
> >> > appropriate then would a PF_MEMPOOL be any better?
> >> 
> >> Why a PF rather than a GFP flag?
> >
> > Well, short answer is that gfp masks are almost depleted.
> 
> Really?  We have 26.
> 
> pagemap has a cute hack to store both GFP flags and other flag bits in
> the one 32 it number per address_space.  'struct address_space' could
> afford an extra 32 number I think.
> 
> radix_tree_root adds 3 'tag' flags to the gfp_mask.
> There is 16bits of free space in radix_tree_node (between 'offset' and
> 'count').  That space on the root node could store a record of which tags
> are set anywhere.  Or would that extra memory de-ref be a killer?

Yes these are reasons why adding new gfp flags is more complicated.

> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> need all those 26).

Well, maybe we are able to remove those hacks, I wouldn't definitely
be opposed.  But right now I am not even convinced that the mempool
specific gfp flags is the right way to go.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1451527

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 28 2016, Michal Hocko wrote:

> On Wed 27-07-16 13:43:35, NeilBrown wrote:
>> On Mon, Jul 25 2016, Michal Hocko wrote:
>> 
>> > On Sat 23-07-16 10:12:24, NeilBrown wrote:
> [...]
>> So should there be a limit on dirty
>> pages in the swap cache just like there is for dirty pages in any
>> filesystem (the max_dirty_ratio thing) ??
>> Maybe there is?
>
> There is no limit AFAIK. We are relying that the reclaim is throttled
> when necessary.

Is that a bit indirect?  It is hard to tell without a clear big-picture.
Something to keep in mind anyway.

>
>> I think we'd end up with cleaner code if we removed the cute-hacks.  And
>> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
>> need all those 26).
>
> Well, maybe we are able to remove those hacks, I wouldn't definitely
> be opposed.  But right now I am not even convinced that the mempool
> specific gfp flags is the right way to go.

I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
transient-allocation gfp flag, which would be quite useful for mempool.

Can you give more details on why using a gfp flag isn't your first choice
for guiding what happens when the system is trying to get a free page
:-?

Thanks,
NeilBrown

[toc] | [prev] | [next] | [standalone]

#1451757 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

On Thu 28-07-16 07:33:19, NeilBrown wrote:
> On Thu, Jul 28 2016, Michal Hocko wrote:
> 
> > On Wed 27-07-16 13:43:35, NeilBrown wrote:
> >> On Mon, Jul 25 2016, Michal Hocko wrote:
> >> 
> >> > On Sat 23-07-16 10:12:24, NeilBrown wrote:
> > [...]
> >> So should there be a limit on dirty
> >> pages in the swap cache just like there is for dirty pages in any
> >> filesystem (the max_dirty_ratio thing) ??
> >> Maybe there is?
> >
> > There is no limit AFAIK. We are relying that the reclaim is throttled
> > when necessary.
> 
> Is that a bit indirect?

Yes it is. Dunno, how much of a problem is that, though.

> It is hard to tell without a clear big-picture.
> Something to keep in mind anyway.
> 
> >
> >> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> >> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> >> need all those 26).
> >
> > Well, maybe we are able to remove those hacks, I wouldn't definitely
> > be opposed.  But right now I am not even convinced that the mempool
> > specific gfp flags is the right way to go.
> 
> I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
> transient-allocation gfp flag, which would be quite useful for mempool.
> 
> Can you give more details on why using a gfp flag isn't your first choice
> for guiding what happens when the system is trying to get a free page
> :-?

If we get rid of throttle_vm_writeout then I guess it might turn out to
be unnecessary. There are other places which will still throttle but I
believe those should be kept regardless of who is doing the allocation
because they are helping the LRU scanning sane. I might be wrong here
and bailing out from the reclaim rather than waiting would turn out
better for some users but I would like to see whether the first approach
works reasonably well.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1455786 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

On Thu, 28 Jul 2016, Michal Hocko wrote:

> > >> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> > >> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> > >> need all those 26).
> > >
> > > Well, maybe we are able to remove those hacks, I wouldn't definitely
> > > be opposed.  But right now I am not even convinced that the mempool
> > > specific gfp flags is the right way to go.
> > 
> > I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
> > transient-allocation gfp flag, which would be quite useful for mempool.
> > 
> > Can you give more details on why using a gfp flag isn't your first choice
> > for guiding what happens when the system is trying to get a free page
> > :-?
> 
> If we get rid of throttle_vm_writeout then I guess it might turn out to
> be unnecessary. There are other places which will still throttle but I
> believe those should be kept regardless of who is doing the allocation
> because they are helping the LRU scanning sane. I might be wrong here
> and bailing out from the reclaim rather than waiting would turn out
> better for some users but I would like to see whether the first approach
> works reasonably well.

If we are swapping to a dm-crypt device, the dm-crypt device is congested 
and the underlying block device is not congested, we should not throttle 
mempool allocations made from the dm-crypt workqueue. Not even a little 
bit.

So, I think, mempool_alloc should set PF_NO_THROTTLE (or 
__GFP_NO_THROTTLE).

Mikulas

> -- 
> Michal Hocko
> SUSE Labs
> 

[toc] | [prev] | [next] | [standalone]

#1455868 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

On Wed 03-08-16 08:53:25, Mikulas Patocka wrote:
> 
> 
> On Thu, 28 Jul 2016, Michal Hocko wrote:
> 
> > > >> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> > > >> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> > > >> need all those 26).
> > > >
> > > > Well, maybe we are able to remove those hacks, I wouldn't definitely
> > > > be opposed.  But right now I am not even convinced that the mempool
> > > > specific gfp flags is the right way to go.
> > > 
> > > I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
> > > transient-allocation gfp flag, which would be quite useful for mempool.
> > > 
> > > Can you give more details on why using a gfp flag isn't your first choice
> > > for guiding what happens when the system is trying to get a free page
> > > :-?
> > 
> > If we get rid of throttle_vm_writeout then I guess it might turn out to
> > be unnecessary. There are other places which will still throttle but I
> > believe those should be kept regardless of who is doing the allocation
> > because they are helping the LRU scanning sane. I might be wrong here
> > and bailing out from the reclaim rather than waiting would turn out
> > better for some users but I would like to see whether the first approach
> > works reasonably well.
> 
> If we are swapping to a dm-crypt device, the dm-crypt device is congested 
> and the underlying block device is not congested, we should not throttle 
> mempool allocations made from the dm-crypt workqueue. Not even a little 
> bit.

But the device congestion is not the only condition required for the
throttling. The pgdat has also be marked congested which means that the
LRU page scanner bumped into dirty/writeback/pg_reclaim pages at the
tail of the LRU. That should only happen if we are rotating LRUs too
quickly. AFAIU the reclaim shouldn't allow free ticket scanning in that
situation.

> So, I think, mempool_alloc should set PF_NO_THROTTLE (or 
> __GFP_NO_THROTTLE).

As I've said earlier that would probably require to bail out from the
reclaim if we detect a potential pgdat congestion. What do you think
Mel?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1456627 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

On Wed, 3 Aug 2016, Michal Hocko wrote:

> On Wed 03-08-16 08:53:25, Mikulas Patocka wrote:
> > 
> > 
> > On Thu, 28 Jul 2016, Michal Hocko wrote:
> > 
> > > > >> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> > > > >> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> > > > >> need all those 26).
> > > > >
> > > > > Well, maybe we are able to remove those hacks, I wouldn't definitely
> > > > > be opposed.  But right now I am not even convinced that the mempool
> > > > > specific gfp flags is the right way to go.
> > > > 
> > > > I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
> > > > transient-allocation gfp flag, which would be quite useful for mempool.
> > > > 
> > > > Can you give more details on why using a gfp flag isn't your first choice
> > > > for guiding what happens when the system is trying to get a free page
> > > > :-?
> > > 
> > > If we get rid of throttle_vm_writeout then I guess it might turn out to
> > > be unnecessary. There are other places which will still throttle but I
> > > believe those should be kept regardless of who is doing the allocation
> > > because they are helping the LRU scanning sane. I might be wrong here
> > > and bailing out from the reclaim rather than waiting would turn out
> > > better for some users but I would like to see whether the first approach
> > > works reasonably well.
> > 
> > If we are swapping to a dm-crypt device, the dm-crypt device is congested 
> > and the underlying block device is not congested, we should not throttle 
> > mempool allocations made from the dm-crypt workqueue. Not even a little 
> > bit.
> 
> But the device congestion is not the only condition required for the
> throttling. The pgdat has also be marked congested which means that the
> LRU page scanner bumped into dirty/writeback/pg_reclaim pages at the
> tail of the LRU. That should only happen if we are rotating LRUs too
> quickly. AFAIU the reclaim shouldn't allow free ticket scanning in that
> situation.

The obvious problem here is that mempool allocations should sleep in 
mempool_alloc() on &pool->wait (until someone returns some entries into 
the mempool), they should not sleep inside the page allocator.

Mikulas

> > So, I think, mempool_alloc should set PF_NO_THROTTLE (or 
> > __GFP_NO_THROTTLE).
> 
> As I've said earlier that would probably require to bail out from the
> reclaim if we detect a potential pgdat congestion. What do you think
> Mel?
> -- 
> Michal Hocko
> SUSE Labs
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web