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

Groups > linux.kernel > #1461145 > 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 Michal Hocko <mhocko@kernel.org> - 2016-08-12 14:40 +0200
    Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-08-13 19:40 +0200
      Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-08-14 12:40 +0200
        Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-08-15 18:20 +0200

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

On Thu 04-08-16 14:49:41, Mikulas Patocka wrote:
> 
> 
> 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.

I agree that mempool_alloc should _primarily_ sleep on their own
throttling mechanism. I am not questioning that. I am just saying that
the page allocator has its own throttling which it relies on and that
cannot be just ignored because that might have other undesirable side
effects. So if the right approach is really to never throttle certain
requests then we have to bail out from a congested nodes/zones as soon
as the congestion is detected.

Now, I would like to see that something like that is _really_ necessary.
I believe that we should simply start with easier part and get rid of
throttle_vm_writeout because that seems like a left over from the past.
If that turns out unsatisfactory and we have clear picture when the
throttling is harmful/suboptimal then we can move on with a more complex
solution. Does this sound like a way forward?

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1461664

On Fri, 12 Aug 2016, Michal Hocko wrote:

> On Thu 04-08-16 14:49:41, Mikulas Patocka wrote:
> 
> > On Wed, 3 Aug 2016, Michal Hocko wrote:
> > 
> > > 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.
> 
> I agree that mempool_alloc should _primarily_ sleep on their own
> throttling mechanism. I am not questioning that. I am just saying that
> the page allocator has its own throttling which it relies on and that
> cannot be just ignored because that might have other undesirable side
> effects. So if the right approach is really to never throttle certain
> requests then we have to bail out from a congested nodes/zones as soon
> as the congestion is detected.
> 
> Now, I would like to see that something like that is _really_ necessary.

Currently, it is not a problem - device mapper reports the device as 
congested only if the underlying physical disks are congested.

But once we change it so that device mapper reports congested state on its 
own (when it has too many bios in progress), this starts being a problem.

I would add PF_NO_THROTTLE or __GFP_NO_THROTTLE to mempool_alloc.

Or - we can prevent the memory reclaim from throttling if we see both 
__GFP_NOMEMALLOC and __GFP_NORETRY - that would be sufficient to detect 
mempool_alloc usage and it wouldn't hurt other __GFP_NORETRY users.

Mikulas

> I believe that we should simply start with easier part and get rid of
> throttle_vm_writeout because that seems like a left over from the past.
> If that turns out unsatisfactory and we have clear picture when the
> throttling is harmful/suboptimal then we can move on with a more complex
> solution. Does this sound like a way forward?
> 
> -- 
> Michal Hocko
> SUSE Labs

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

#1461780

On Sat 13-08-16 13:34:29, Mikulas Patocka wrote:
> 
> 
> On Fri, 12 Aug 2016, Michal Hocko wrote:
> 
> > On Thu 04-08-16 14:49:41, Mikulas Patocka wrote:
> > 
> > > On Wed, 3 Aug 2016, Michal Hocko wrote:
> > > 
> > > > 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.
> > 
> > I agree that mempool_alloc should _primarily_ sleep on their own
> > throttling mechanism. I am not questioning that. I am just saying that
> > the page allocator has its own throttling which it relies on and that
> > cannot be just ignored because that might have other undesirable side
> > effects. So if the right approach is really to never throttle certain
> > requests then we have to bail out from a congested nodes/zones as soon
> > as the congestion is detected.
> > 
> > Now, I would like to see that something like that is _really_ necessary.
> 
> Currently, it is not a problem - device mapper reports the device as 
> congested only if the underlying physical disks are congested.
> 
> But once we change it so that device mapper reports congested state on its 
> own (when it has too many bios in progress), this starts being a problem.

OK, can we wait until it starts becoming a real problem and solve it
appropriately then?

I will repost the patch which removes thottle_vm_pageout in the meantime
as it doesn't seem to be needed anymore.

-- 
Michal Hocko
SUSE Labs

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

#1462964

On Sun, 14 Aug 2016, Michal Hocko wrote:

> On Sat 13-08-16 13:34:29, Mikulas Patocka wrote:
> > 
> > 
> > On Fri, 12 Aug 2016, Michal Hocko wrote:
> > 
> > > On Thu 04-08-16 14:49:41, Mikulas Patocka wrote:
> > > 
> > > > On Wed, 3 Aug 2016, Michal Hocko wrote:
> > > > 
> > > > > 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.
> > > 
> > > I agree that mempool_alloc should _primarily_ sleep on their own
> > > throttling mechanism. I am not questioning that. I am just saying that
> > > the page allocator has its own throttling which it relies on and that
> > > cannot be just ignored because that might have other undesirable side
> > > effects. So if the right approach is really to never throttle certain
> > > requests then we have to bail out from a congested nodes/zones as soon
> > > as the congestion is detected.
> > > 
> > > Now, I would like to see that something like that is _really_ necessary.
> > 
> > Currently, it is not a problem - device mapper reports the device as 
> > congested only if the underlying physical disks are congested.
> > 
> > But once we change it so that device mapper reports congested state on its 
> > own (when it has too many bios in progress), this starts being a problem.
> 
> OK, can we wait until it starts becoming a real problem and solve it
> appropriately then?

I don't like the idea to deliberately introduce some code that triggers 
this bug into device mapper, then wait until some user hits the bug and 
then fix the bug.

If the VM throttles mempool allocations when the swap device is congested 
- than I won't report the device as congested in the device mapper.

Mikulas

> I will repost the patch which removes thottle_vm_pageout in the meantime
> as it doesn't seem to be needed anymore.
> 
> -- 
> Michal Hocko
> SUSE Labs
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web