__GFP_NO_LOCKS

Limitations and concerns (Main part)
====================================
The current memmory-allocation interface presents to following difficulties that this patch is designed to overcome:

a) If built with CONFIG_PROVE_RAW_LOCK_NESTING, the lockdep will complain about violation("BUG: Invalid wait context") of the nesting rules. It does the raw_spinlock vs. spinlock nesting checks, i.e. it is not legal to acquire a spinlock_t while holding a raw_spinlock_t.

   Internally the kfree_rcu() uses raw_spinlock_t(in rcu-dev branch)
   whereas the "page allocator" internally deals with spinlock_t to
   access to its zones. The code also can be broken from higher level
   of view:
   <snip>
       raw_spin_lock(&some_lock);
       kfree_rcu(some_pointer, some_field_offset);
   <snip>

b) If built with CONFIG_PREEMPT_RT. Please note, in that case spinlock_t is converted into sleepable variant. Invoking the page allocator from atomic contexts leads to "BUG: scheduling while atomic".

Proposal
========
1) Make GFP_* that ensures that the allocator returns NULL rather than acquire its own spinlock_t. Having such flag will address a and b limitations described above. It will also make the kfree_rcu() code common for RT and regular kernel, more clean, less handling corner cases and reduce the code size.

[...]

Quoted 11 lines

Limitations and concerns (Main part)
====================================
The current memmory-allocation interface presents to following difficulties that this patch is designed to overcome:

a) If built with CONFIG_PROVE_RAW_LOCK_NESTING, the lockdep will complain about violation("BUG: Invalid wait context") of the nesting rules. It does the raw_spinlock vs. spinlock nesting checks, i.e. it is not legal to acquire a spinlock_t while holding a raw_spinlock_t.

Quoted 8 lines

   Internally the kfree_rcu() uses raw_spinlock_t(in rcu-dev branch)
   whereas the "page allocator" internally deals with spinlock_t to
   access to its zones. The code also can be broken from higher level
   of view:
   <snip>
       raw_spin_lock(&some_lock);
       kfree_rcu(some_pointer, some_field_offset);
   <snip>

Is there any fundamental problem to make zone raw_spin_lock?

Quoted 3 lines

b) If built with CONFIG_PREEMPT_RT. Please note, in that case spinlock_t is converted into sleepable variant. Invoking the page allocator from atomic contexts leads to "BUG: scheduling while atomic".

[...]

Quoted 7 lines

Proposal
========
1) Make GFP_* that ensures that the allocator returns NULL rather than acquire its own spinlock_t. Having such flag will address a and b limitations described above. It will also make the kfree_rcu() code common for RT and regular kernel, more clean, less handling corner cases and reduce the code size.

I do not think this is a good idea. Single purpose gfp flags that tend to heavily depend on the current implementation of the page allocator have turned out to be problematic. Users used to misunderstand their meaning resulting in a lot of abuse which was not trivial to remove. This flag seem to fall into exactly this sort of category. If there is a problem in nesting then that should be addressed rather than a new flag exported IMHO. If that is absolutely not possible for some reason then we can try to figure out what to do but that really need a very strong justification.

Quoted 22 lines · Uladzislau Rezki (Sony)

[...]

Quoted 19 lines

Limitations and concerns (Main part)
====================================
The current memmory-allocation interface presents to following difficulties that this patch is designed to overcome:

a) If built with CONFIG_PROVE_RAW_LOCK_NESTING, the lockdep will complain about violation("BUG: Invalid wait context") of the nesting rules. It does the raw_spinlock vs. spinlock nesting checks, i.e. it is not legal to acquire a spinlock_t while holding a raw_spinlock_t.

   Internally the kfree_rcu() uses raw_spinlock_t(in rcu-dev branch)
   whereas the "page allocator" internally deals with spinlock_t to
   access to its zones. The code also can be broken from higher level
   of view:
   <snip>
       raw_spin_lock(&some_lock);
       kfree_rcu(some_pointer, some_field_offset);
   <snip>

Quoted 2 lines

Is there any fundamental problem to make zone raw_spin_lock?

Good point. Converting a regular spinlock to the raw_* variant can solve an issue and to me it seems partly reasonable. Because there are other questions if we do it:

a) what to do with kswapd and "wake-up path" that uses sleepable lock:

    wakeup_kswapd() -> wake_up_interruptible(&pgdat->kswapd_wait).

b) How RT people reacts on it? I guess they will no be happy.

As i described before, calling the __get_free_page(0) with 0 as argument will solve the (a). How correctly is it? From my point of view the logic that bypass the wakeup path should be explicitly defined.

Or we can enter the allocator with (__GFP_HIGH|__GFP_ATOMIC) that bypass the __GFP_KSWAPD_RECLAIM as well.

Any thoughts here? Please comment.

Having proposed flag will not heart RT latency and solve all concerns.

Quoted 14 lines

Quoted 3 lines

b) If built with CONFIG_PREEMPT_RT. Please note, in that case spinlock_t is converted into sleepable variant. Invoking the page allocator from atomic contexts leads to "BUG: scheduling while atomic".

[...]

Quoted 7 lines

Proposal
========
1) Make GFP_* that ensures that the allocator returns NULL rather than acquire its own spinlock_t. Having such flag will address a and b limitations described above. It will also make the kfree_rcu() code common for RT and regular kernel, more clean, less handling corner cases and reduce the code size.

Quoted 10 lines

I do not think this is a good idea. Single purpose gfp flags that tend to heavily depend on the current implementation of the page allocator have turned out to be problematic. Users used to misunderstand their meaning resulting in a lot of abuse which was not trivial to remove. This flag seem to fall into exactly this sort of category. If there is a problem in nesting then that should be addressed rather than a new flag exported IMHO. If that is absolutely not possible for some reason then we can try to figure out what to do but that really need a very strong justification.

The problem that i see is we can not use the page allocator from atomic contexts, what is our case:

<snip>

    local_irq_save(flags) or preempt_disable() or raw_spinlock();
    __get_free_page(GFP_ATOMIC);

<snip>

So if we can convert the page allocator to raw_* lock it will be appreciated, at least from our side, IMHO, not from RT one. But as i stated above we need to sort raised questions out if converting is done.

What is your view?

On Mon 10-08-20 18:07:39, Uladzislau Rezki wrote:

Quoted 12 lines

The problem that i see is we can not use the page allocator from atomic contexts, what is our case:

<snip>

    local_irq_save(flags) or preempt_disable() or raw_spinlock();
    __get_free_page(GFP_ATOMIC);

<snip>

So if we can convert the page allocator to raw_* lock it will be appreciated, at least from our side, IMHO, not from RT one. But as i stated above we need to sort raised questions out if converting is done.

Quoted 1 line

What is your view?

To me it would make more sense to support atomic allocations also for the RT tree. Having both GFP_NOWAIT and GFP_ATOMIC which do not really work for atomic context in RT sounds subtle and wrong.

Quoted 1 line · Michal Hocko

On Mon 10-08-20 18:07:39, Uladzislau Rezki wrote:

[...]

Quoted 12 lines

Quoted 12 lines

The problem that i see is we can not use the page allocator from atomic contexts, what is our case:

<snip>

    local_irq_save(flags) or preempt_disable() or raw_spinlock();
    __get_free_page(GFP_ATOMIC);

<snip>

So if we can convert the page allocator to raw_* lock it will be appreciated, at least from our side, IMHO, not from RT one. But as i stated above we need to sort raised questions out if converting is done.

Quoted 5 lines

Quoted 1 line

What is your view?

To me it would make more sense to support atomic allocations also for the RT tree. Having both GFP_NOWAIT and GFP_ATOMIC which do not really work for atomic context in RT sounds subtle and wrong.

I was thinking about this some more. I still think the above would be a reasonable goal we should try to achieve. If for not other then for future maintainability (especially after the RT patchset is merged). I have tried to search for any known problems/attempts to make zone->lock raw but couldn't find anything. Maybe somebody more involved in RT world have something to say about that.

Anyway, if the zone->lock is not a good fit for raw_spin_lock then the only way I can see forward is to detect real (RT) atomic contexts and bail out early before taking the lock in the allocator for NOWAIT/ATOMIC requests.

Quoted 21 lines · Michal Hocko

Quoted 1 line · Michal Hocko

On Mon 10-08-20 18:07:39, Uladzislau Rezki wrote:

[...]

Quoted 12 lines

Quoted 12 lines

The problem that i see is we can not use the page allocator from atomic contexts, what is our case:

<snip>

    local_irq_save(flags) or preempt_disable() or raw_spinlock();
    __get_free_page(GFP_ATOMIC);

<snip>

So if we can convert the page allocator to raw_* lock it will be appreciated, at least from our side, IMHO, not from RT one. But as i stated above we need to sort raised questions out if converting is done.

Quoted 5 lines

Quoted 1 line

What is your view?

To me it would make more sense to support atomic allocations also for the RT tree. Having both GFP_NOWAIT and GFP_ATOMIC which do not really work for atomic context in RT sounds subtle and wrong.

Quoted 7 lines

I was thinking about this some more. I still think the above would be a reasonable goal we should try to achieve. If for not other then for future maintainability (especially after the RT patchset is merged). I have tried to search for any known problems/attempts to make zone->lock raw but couldn't find anything. Maybe somebody more involved in RT world have something to say about that.

I tried yesterday to convert zone->lock. See below files i had to modify:
<snip>

        modified:   include/linux/mmzone.h
        modified:   mm/compaction.c
        modified:   mm/memory_hotplug.c
        modified:   mm/page_alloc.c
        modified:   mm/page_isolation.c
        modified:   mm/page_reporting.c
        modified:   mm/shuffle.c
        modified:   mm/vmscan.c
        modified:   mm/vmstat.c

<snip>

There is one more lock, that is zone->lru_lock one. Both zone->lock and this one intersect between each other. If the lru_lock can be nested under zone->lock it should be converted as well. But i need to analyze it farther. There are two wrapper functions which are used as common interface to lock/unlock both locks. See compact_lock_irqsave()/compact_unlock_should_abort_lru() in the mm/compaction.c.

Any thoughts here?

Anyway i tried to convert only zone->lock and use page allocator passing there gfp_mask=0 as argument. So it works. CONFIG_PROVE_RAW_LOCK_NESTING does not complain about any "bad" lock nesting.

Quoted 5 lines

Anyway, if the zone->lock is not a good fit for raw_spin_lock then the only way I can see forward is to detect real (RT) atomic contexts and bail out early before taking the lock in the allocator for NOWAIT/ATOMIC requests.

For RT kernel we can detect it for sure. preemtable() works just fine there, i.e. we can identify the context we are currently in.

Quoted 1 line · Uladzislau Rezki

On Tue, Aug 11, 2020 at 10:19:17AM +0200, Michal Hocko wrote:

Quoted 5 lines

Quoted 5 lines

Anyway, if the zone->lock is not a good fit for raw_spin_lock then the only way I can see forward is to detect real (RT) atomic contexts and bail out early before taking the lock in the allocator for NOWAIT/ATOMIC requests.

This is similar what i have done with mm: Add __GFP_NO_LOCKS flag. I just did it for order-0 pages(other paths are impossible) and made it common for any kernel.

Because when you say "bail out early" i suspect that we would like to check the per-cpu-list cache.

Quoted 2 lines · Uladzislau Rezki

Quoted 1 line · Uladzislau Rezki

On Tue, Aug 11, 2020 at 10:19:17AM +0200, Michal Hocko wrote:

[...]

Quoted 9 lines

Quoted 5 lines

Quoted 5 lines

Anyway, if the zone->lock is not a good fit for raw_spin_lock then the only way I can see forward is to detect real (RT) atomic contexts and bail out early before taking the lock in the allocator for NOWAIT/ATOMIC requests.

This is similar what i have done with mm: Add __GFP_NO_LOCKS flag. I just did it for order-0 pages(other paths are impossible) and made it common for any kernel.

Quoted 2 lines

Because when you say "bail out early" i suspect that we would like to check the per-cpu-list cache.

Bail out early means to do as much as possible until a raw non-compliant lock has to be taken.

Quoted 21 lines · Michal Hocko

Quoted 1 line · Michal Hocko

On Mon 10-08-20 18:07:39, Uladzislau Rezki wrote:

[...]

Quoted 12 lines

Quoted 12 lines

The problem that i see is we can not use the page allocator from atomic contexts, what is our case:

<snip>

    local_irq_save(flags) or preempt_disable() or raw_spinlock();
    __get_free_page(GFP_ATOMIC);

<snip>

So if we can convert the page allocator to raw_* lock it will be appreciated, at least from our side, IMHO, not from RT one. But as i stated above we need to sort raised questions out if converting is done.

Quoted 5 lines

Quoted 1 line

What is your view?

To me it would make more sense to support atomic allocations also for the RT tree. Having both GFP_NOWAIT and GFP_ATOMIC which do not really work for atomic context in RT sounds subtle and wrong.

Quoted 7 lines

I was thinking about this some more. I still think the above would be a reasonable goal we should try to achieve. If for not other then for future maintainability (especially after the RT patchset is merged). I have tried to search for any known problems/attempts to make zone->lock raw but couldn't find anything. Maybe somebody more involved in RT world have something to say about that.

I tried yesterday to convert zone->lock. See below files i had to modify:
<snip>

        modified:   include/linux/mmzone.h
        modified:   mm/compaction.c
        modified:   mm/memory_hotplug.c
        modified:   mm/page_alloc.c
        modified:   mm/page_isolation.c
        modified:   mm/page_reporting.c
        modified:   mm/shuffle.c
        modified:   mm/vmscan.c
        modified:   mm/vmstat.c

<snip>

There is one more lock, that is zone->lru_lock one. Both zone->lock and this one intersect between each other. If the lru_lock can be nested under zone->lock it should be converted as well. But i need to analyze it farther. There are two wrapper functions which are used as common interface to lock/unlock both locks. See compact_lock_irqsave()/compact_unlock_should_abort_lru() in the mm/compaction.c.

Any thoughts here?

Anyway i tried to convert only zone->lock and use page allocator passing there gfp_mask=0 as argument. So it works. CONFIG_PROVE_RAW_LOCK_NESTING does not complain about any "bad" lock nesting.

Quoted 5 lines

Anyway, if the zone->lock is not a good fit for raw_spin_lock then the only way I can see forward is to detect real (RT) atomic contexts and bail out early before taking the lock in the allocator for NOWAIT/ATOMIC requests.

For RT kernel we can detect it for sure. preemtable() works just fine there, i.e. we can identify the context we are currently in.

Quoted 42 lines · Uladzislau Rezki

Quoted 21 lines · Michal Hocko

Quoted 1 line · Michal Hocko

On Mon 10-08-20 18:07:39, Uladzislau Rezki wrote:

[...]

Quoted 12 lines

Quoted 12 lines

The problem that i see is we can not use the page allocator from atomic contexts, what is our case:

<snip>

    local_irq_save(flags) or preempt_disable() or raw_spinlock();
    __get_free_page(GFP_ATOMIC);

<snip>

So if we can convert the page allocator to raw_* lock it will be appreciated, at least from our side, IMHO, not from RT one. But as i stated above we need to sort raised questions out if converting is done.

Quoted 5 lines

Quoted 1 line

What is your view?

To me it would make more sense to support atomic allocations also for the RT tree. Having both GFP_NOWAIT and GFP_ATOMIC which do not really work for atomic context in RT sounds subtle and wrong.

Quoted 7 lines

I was thinking about this some more. I still think the above would be a reasonable goal we should try to achieve. If for not other then for future maintainability (especially after the RT patchset is merged). I have tried to search for any known problems/attempts to make zone->lock raw but couldn't find anything. Maybe somebody more involved in RT world have something to say about that.

I tried yesterday to convert zone->lock. See below files i had to modify:
<snip>

        modified:   include/linux/mmzone.h
        modified:   mm/compaction.c
        modified:   mm/memory_hotplug.c
        modified:   mm/page_alloc.c
        modified:   mm/page_isolation.c
        modified:   mm/page_reporting.c
        modified:   mm/shuffle.c
        modified:   mm/vmscan.c
        modified:   mm/vmstat.c

<snip>

Quoted 8 lines

There is one more lock, that is zone->lru_lock one. Both zone->lock and this one intersect between each other. If the lru_lock can be nested under zone->lock it should be converted as well. But i need to analyze it farther. There are two wrapper functions which are used as common interface to lock/unlock both locks. See compact_lock_irqsave()/compact_unlock_should_abort_lru() in the mm/compaction.c.

Any thoughts here?

I am not an expert on compaction. Vlastimil would know better. My thinking was that zone->lock is a tail lock but compaction/page isolation might be doing something I am not aware of right now.

Quoted 11 lines

Anyway i tried to convert only zone->lock and use page allocator passing there gfp_mask=0 as argument. So it works. CONFIG_PROVE_RAW_LOCK_NESTING does not complain about any "bad" lock nesting.

Quoted 5 lines

Anyway, if the zone->lock is not a good fit for raw_spin_lock then the only way I can see forward is to detect real (RT) atomic contexts and bail out early before taking the lock in the allocator for NOWAIT/ATOMIC requests.

For RT kernel we can detect it for sure. preemtable() works just fine there, i.e. we can identify the context we are currently in.

In previous email I didn't mention why I prefer full NOWAIT semantic over rt specific bailouts. There are users making NOWAIT allocation attempts as an opportunistic allocation request which is OK to fail as they have a fallback to go through. This would imply they would prefer to know this ASAP rather then get blocked and sleep. A lack of reports for PREEMPT_RT would suggest that nobody has noticed as this though.

Quoted 28 lines · Uladzislau Rezki

Quoted 22 lines · Uladzislau Rezki (Sony)

[...]

Quoted 11 lines

Limitations and concerns (Main part)
====================================
The current memmory-allocation interface presents to following difficulties that this patch is designed to overcome:

a) If built with CONFIG_PROVE_RAW_LOCK_NESTING, the lockdep will complain about violation("BUG: Invalid wait context") of the nesting rules. It does the raw_spinlock vs. spinlock nesting checks, i.e. it is not legal to acquire a spinlock_t while holding a raw_spinlock_t.

Quoted 8 lines

   Internally the kfree_rcu() uses raw_spinlock_t(in rcu-dev branch)
   whereas the "page allocator" internally deals with spinlock_t to
   access to its zones. The code also can be broken from higher level
   of view:
   <snip>
       raw_spin_lock(&some_lock);
       kfree_rcu(some_pointer, some_field_offset);
   <snip>

Quoted 2 lines

Is there any fundamental problem to make zone raw_spin_lock?

Good point. Converting a regular spinlock to the raw_* variant can solve an issue and to me it seems partly reasonable. Because there are other questions if we do it:

Quoted 2 lines

a) what to do with kswapd and "wake-up path" that uses sleepable lock:

    wakeup_kswapd() -> wake_up_interruptible(&pgdat->kswapd_wait).

If there is no RT friendly variant for waking up process from the atomic context then we might need to special case this for the RT tree.

Quoted 1 line

b) How RT people reacts on it? I guess they will no be happy.

zone->lock should be held for a very limited amount of time.

Quoted 3 lines

As i described before, calling the __get_free_page(0) with 0 as argument will solve the (a). How correctly is it? From my point of view the logic that bypass the wakeup path should be explicitly defined.

gfp_mask == 0 is GFP_NOWAIT (aka an atomic allocation request) which doesn't wake up kswapd. So if the wakeup is a problem then this would be a way to go.

Quoted 2 lines

Or we can enter the allocator with (__GFP_HIGH|__GFP_ATOMIC) that bypass the __GFP_KSWAPD_RECLAIM as well.

This would be an alternative which consumes memory reserves. Is this really needed for the particular case?

Quoted 41 lines

Any thoughts here? Please comment.

Having proposed flag will not heart RT latency and solve all concerns.

Quoted 14 lines

Quoted 3 lines

b) If built with CONFIG_PREEMPT_RT. Please note, in that case spinlock_t is converted into sleepable variant. Invoking the page allocator from atomic contexts leads to "BUG: scheduling while atomic".

[...]

Quoted 7 lines

Proposal
========
1) Make GFP_* that ensures that the allocator returns NULL rather than acquire its own spinlock_t. Having such flag will address a and b limitations described above. It will also make the kfree_rcu() code common for RT and regular kernel, more clean, less handling corner cases and reduce the code size.

Quoted 10 lines

I do not think this is a good idea. Single purpose gfp flags that tend to heavily depend on the current implementation of the page allocator have turned out to be problematic. Users used to misunderstand their meaning resulting in a lot of abuse which was not trivial to remove. This flag seem to fall into exactly this sort of category. If there is a problem in nesting then that should be addressed rather than a new flag exported IMHO. If that is absolutely not possible for some reason then we can try to figure out what to do but that really need a very strong justification.

The problem that i see is we can not use the page allocator from atomic contexts, what is our case:

<snip>

    local_irq_save(flags) or preempt_disable() or raw_spinlock();
    __get_free_page(GFP_ATOMIC);

<snip>

So if we can convert the page allocator to raw_* lock it will be appreciated, at least from our side, IMHO, not from RT one. But as i stated above we need to sort raised questions out if converting is done.

Quoted 1 line

What is your view?

To me it would make more sense to support atomic allocations also for the RT tree. Having both GFP_NOWAIT and GFP_ATOMIC which do not really work for atomic context in RT sounds subtle and wrong.

Quoted 2 lines · Michal Hocko

Quoted 1 line · Uladzislau Rezki

Quoted 1 line · Uladzislau Rezki (Sony)

Quoted 8 lines

Quoted 3 lines

As i described before, calling the __get_free_page(0) with 0 as argument will solve the (a). How correctly is it? From my point of view the logic that bypass the wakeup path should be explicitly defined.

gfp_mask == 0 is GFP_NOWAIT (aka an atomic allocation request) which doesn't wake up kswapd. So if the wakeup is a problem then this would be a way to go.

What do you mean Michal? gfp_mask 0 != GFP_NOWAIT:

#define GFP_NOWAIT (__GFP_KSWAPD_RECLAIM)

it does wakeup of the kswapd. Or am i missing something? Please comment. If we are about to avoid the kswapd, should we define something special?

#define GFP_NOWWAKE_KSWAPD 0

Quoted 3 lines · Uladzislau Rezki

Quoted 2 lines · Michal Hocko

Quoted 1 line · Uladzislau Rezki

Quoted 1 line · Uladzislau Rezki (Sony)

[...]

Quoted 15 lines

Quoted 8 lines

Quoted 3 lines

As i described before, calling the __get_free_page(0) with 0 as argument will solve the (a). How correctly is it? From my point of view the logic that bypass the wakeup path should be explicitly defined.

gfp_mask == 0 is GFP_NOWAIT (aka an atomic allocation request) which doesn't wake up kswapd. So if the wakeup is a problem then this would be a way to go.

What do you mean Michal? gfp_mask 0 != GFP_NOWAIT:

#define GFP_NOWAIT (__GFP_KSWAPD_RECLAIM)

it does wakeup of the kswapd. Or am i missing something? Please comment. If we are about to avoid the kswapd, should we define something special?

Quoted 1 line

#define GFP_NOWWAKE_KSWAPD 0

Sorry, I was more cryptic than necessary. What I meant is that GFP_NOWAIT is the basic non-sleepable allocation. It does wake up kswapd but a lack of it can be expressed GFP_NOWAIT & ~__GFP_KSWAPD_RECLAIM which is 0, now. The mouthfull variant is better for future maintainability.

Quoted 7 lines · Uladzislau Rezki

Quoted 3 lines · Uladzislau Rezki (Sony)

Is there any fundamental problem to make zone raw_spin_lock?

Good point. Converting a regular spinlock to the raw_* variant can solve an issue and to me it seems partly reasonable. Because there are other questions if we do it:

Quoted 2 lines

a) what to do with kswapd and "wake-up path" that uses sleepable lock:

    wakeup_kswapd() -> wake_up_interruptible(&pgdat->kswapd_wait).

If there is no RT friendly variant for waking up process from the atomic context then we might need to special case this for the RT tree.

Quoted 1 line

b) How RT people reacts on it? I guess they will no be happy.

zone->lock should be held for a very limited amount of time.

Quoted 3 lines

As i described before, calling the __get_free_page(0) with 0 as argument will solve the (a). How correctly is it? From my point of view the logic that bypass the wakeup path should be explicitly defined.

gfp_mask == 0 is GFP_NOWAIT (aka an atomic allocation request) which doesn't wake up kswapd. So if the wakeup is a problem then this would be a way to go.

To me it would make more sense to support atomic allocations also for the RT tree. Having both GFP_NOWAIT and GFP_ATOMIC which do not really work for atomic context in RT sounds subtle and wrong.

Quoted 1 line · Michal Hocko

zone->lock should be held for a very limited amount of time.

Emphasis on should. free_pcppages_bulk() can hold it for quite some time when a large amount of pages are purged. We surely would have converted it to a raw lock long time ago otherwise.

For regular enterprise stuff a few hundred microseconds might qualify as a limited amount of time. For advanced RT applications that's way beyond tolerable..

Quoted 10 lines · Thomas Gleixner

Quoted 1 line · Michal Hocko

zone->lock should be held for a very limited amount of time.

Emphasis on should. free_pcppages_bulk() can hold it for quite some time when a large amount of pages are purged. We surely would have converted it to a raw lock long time ago otherwise.

For regular enterprise stuff a few hundred microseconds might qualify as a limited amount of time. For advanced RT applications that's way beyond tolerable..

Sebastian just tried with zone lock converted to a raw lock and maximum latencies go up by a factor of 7 when putting a bit of stress on the memory subsytem. Just a regular kernel compile kicks them up by a factor of 5. Way out of tolerance.

We'll have a look whether it's solely free_pcppages_bulk() and if so we could get away with dropping the lock in the loop.

Quoted 17 lines · Thomas Gleixner

Quoted 10 lines · Thomas Gleixner

Quoted 1 line · Michal Hocko

zone->lock should be held for a very limited amount of time.

Emphasis on should. free_pcppages_bulk() can hold it for quite some time when a large amount of pages are purged. We surely would have converted it to a raw lock long time ago otherwise.

For regular enterprise stuff a few hundred microseconds might qualify as a limited amount of time. For advanced RT applications that's way beyond tolerable..

Sebastian just tried with zone lock converted to a raw lock and maximum latencies go up by a factor of 7 when putting a bit of stress on the memory subsytem. Just a regular kernel compile kicks them up by a factor of 5. Way out of tolerance.

Quoted 2 lines

We'll have a look whether it's solely free_pcppages_bulk() and if so we could get away with dropping the lock in the loop.

So even on !RT and just doing a kernel compile the time spent in free_pcppages_bulk() is up to 270 usec.

It's not only the loop which processes a large pile of pages, part of it is caused by lock contention on zone->lock. Dropping the lock after a processing a couple of pages does not make it much better if enough CPUs are contending on the lock.

Quoted 12 lines · Thomas Gleixner

Quoted 10 lines · Thomas Gleixner

Quoted 1 line · Michal Hocko

zone->lock should be held for a very limited amount of time.

Emphasis on should. free_pcppages_bulk() can hold it for quite some time when a large amount of pages are purged. We surely would have converted it to a raw lock long time ago otherwise.

For regular enterprise stuff a few hundred microseconds might qualify as a limited amount of time. For advanced RT applications that's way beyond tolerable..

Quoted 7 lines

Sebastian just tried with zone lock converted to a raw lock and maximum latencies go up by a factor of 7 when putting a bit of stress on the memory subsytem. Just a regular kernel compile kicks them up by a factor of 5. Way out of tolerance.

We'll have a look whether it's solely free_pcppages_bulk() and if so we could get away with dropping the lock in the loop.

So even on !RT and just doing a kernel compile the time spent in free_pcppages_bulk() is up to 270 usec.

It's not only the loop which processes a large pile of pages, part of it is caused by lock contention on zone->lock. Dropping the lock after a processing a couple of pages does not make it much better if enough CPUs are contending on the lock.

Quoted 8 lines · Michal Hocko

Quoted 7 lines · Uladzislau Rezki

Quoted 3 lines · Uladzislau Rezki (Sony)

Is there any fundamental problem to make zone raw_spin_lock?

Good point. Converting a regular spinlock to the raw_* variant can solve an issue and to me it seems partly reasonable. Because there are other questions if we do it:

Quoted 5 lines

Quoted 2 lines

a) what to do with kswapd and "wake-up path" that uses sleepable lock:

    wakeup_kswapd() -> wake_up_interruptible(&pgdat->kswapd_wait).

If there is no RT friendly variant for waking up process from the atomic context then we might need to special case this for the RT tree.

That's a solvable problem.

Quoted 3 lines

Quoted 1 line

b) How RT people reacts on it? I guess they will no be happy.

zone->lock should be held for a very limited amount of time.

Emphasis on should. free_pcppages_bulk() can hold it for quite some time when a large amount of pages are purged. We surely would have converted it to a raw lock long time ago otherwise.

For regular enterprise stuff a few hundred microseconds might qualify as a limited amount of time. For advanced RT applications that's way beyond tolerable..

Quoted 7 lines

Quoted 3 lines

As i described before, calling the __get_free_page(0) with 0 as argument will solve the (a). How correctly is it? From my point of view the logic that bypass the wakeup path should be explicitly defined.

gfp_mask == 0 is GFP_NOWAIT (aka an atomic allocation request) which doesn't wake up kswapd. So if the wakeup is a problem then this would be a way to go.

The wakeup is the least of my worries.

Quoted 3 lines

To me it would make more sense to support atomic allocations also for the RT tree. Having both GFP_NOWAIT and GFP_ATOMIC which do not really work for atomic context in RT sounds subtle and wrong.

Well, no. RT moves almost everything out of atomic context which means that GFP_ATOMIC is pretty meanlingless on a RT kernel. RT sacrifies performance for determinism. It's a known tradeoff.

Now RCU creates a new thing which enforces to make page allocation in atomic context possible on RT. What for?

What's the actual use case in truly atomic context for this new thing on an RT kernel?

The actual RCU code disabling interrupts is an implementation detail which can easily be mitigated with a local lock.

Quoted 5 lines · Thomas Gleixner

Now RCU creates a new thing which enforces to make page allocation in atomic context possible on RT. What for?

What's the actual use case in truly atomic context for this new thing on an RT kernel?

It is not just RT kernels. CONFIG_PROVE_RAW_LOCK_NESTING=y propagates this constraint to all configurations, and a patch in your new favorite subsystem really did trigger this lockdep check in a non-RT kernel.

Quoted 2 lines

The actual RCU code disabling interrupts is an implementation detail which can easily be mitigated with a local lock.

In this case, we are in raw-spinlock context on entry to kfree_rcu().

Where?

Quoted 11 lines · Paul E. McKenney

Quoted 5 lines · Thomas Gleixner

Now RCU creates a new thing which enforces to make page allocation in atomic context possible on RT. What for?

What's the actual use case in truly atomic context for this new thing on an RT kernel?

It is not just RT kernels. CONFIG_PROVE_RAW_LOCK_NESTING=y propagates this constraint to all configurations, and a patch in your new favorite subsystem really did trigger this lockdep check in a non-RT kernel.

Quoted 4 lines

Quoted 2 lines

The actual RCU code disabling interrupts is an implementation detail which can easily be mitigated with a local lock.

In this case, we are in raw-spinlock context on entry to kfree_rcu().

Where?

Quoted 12 lines · Thomas Gleixner

Quoted 11 lines · Paul E. McKenney

Quoted 5 lines · Thomas Gleixner

Now RCU creates a new thing which enforces to make page allocation in atomic context possible on RT. What for?

What's the actual use case in truly atomic context for this new thing on an RT kernel?

It is not just RT kernels. CONFIG_PROVE_RAW_LOCK_NESTING=y propagates this constraint to all configurations, and a patch in your new favorite subsystem really did trigger this lockdep check in a non-RT kernel.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

The actual RCU code disabling interrupts is an implementation detail which can easily be mitigated with a local lock.

In this case, we are in raw-spinlock context on entry to kfree_rcu().

Where?

Some BPF code that needs to process and free a list. As noted above, this is a patch rather than something that is already in mainline. Not surprising, though, given call_rcu() invocations in similar contexts.

Yes, we can perhaps rework all current and future callers to avoid invoking both call_rcu() and kfree_rcu() from raw atomic context, but the required change to permit this is quite a bit simpler.

Quoted 11 lines · Paul E. McKenney

Quoted 5 lines · Thomas Gleixner

Now RCU creates a new thing which enforces to make page allocation in atomic context possible on RT. What for?

What's the actual use case in truly atomic context for this new thing on an RT kernel?

It is not just RT kernels. CONFIG_PROVE_RAW_LOCK_NESTING=y propagates this constraint to all configurations, and a patch in your new favorite subsystem really did trigger this lockdep check in a non-RT kernel.

Quoted 4 lines

Quoted 2 lines

The actual RCU code disabling interrupts is an implementation detail which can easily be mitigated with a local lock.

In this case, we are in raw-spinlock context on entry to kfree_rcu().

Where?

Quoted 15 lines · Thomas Gleixner

Quoted 14 lines · Paul E. McKenney

Quoted 5 lines · Thomas Gleixner

Now RCU creates a new thing which enforces to make page allocation in atomic context possible on RT. What for?

What's the actual use case in truly atomic context for this new thing on an RT kernel?

It is not just RT kernels. CONFIG_PROVE_RAW_LOCK_NESTING=y propagates this constraint to all configurations, and a patch in your new favorite subsystem really did trigger this lockdep check in a non-RT kernel.

Quoted 2 lines

The actual RCU code disabling interrupts is an implementation detail which can easily be mitigated with a local lock.

Quoted 3 lines

Quoted 1 line

In this case, we are in raw-spinlock context on entry to kfree_rcu().

Where?

And aside of the where, wasn't kfree_rcu() from within raw spinlock held regions possible all the time? Either I'm missing something or you are fundamentally changing RCU internals. kfree_rcu() saved RT in various ways where invoking kfree() was just not an option. Confused...

Hence Ulad's work on kfree_rcu(). The approach is to allocate a page-sized array to hold all the pointers, then fill in the rest of these pointers on each subsequent kfree_rcu() call. These arrays of pointers also allows use of kfree_bulk() instead of kfree(), which can improve performance yet more. It is no big deal if kfree_rcu()'s allocation attempts fail occasionally because it can simply fall back to the old linked-list approach. And given that the various lockless caches in the memory allocator are almost never empty, in theory life is good.

But in practice, mainline now has CONFIG_PROVE_RAW_LOCK_NESTING, and for good reason -- this Kconfig option makes it at least a little bit harder for mainline developers to mess up RT. But with CONFIG_PROVE_RAW_LOCK_NESTING=y and lockdep enabled, mainline will now sometimes complain if you invoke kfree_rcu() while holding a raw spinlock. This happens when kfree_rcu() needs to invoke the memory allocator and the memory allocator's caches are empty, thus resulting in the memory allocator attempting to acquire a non-raw spinlock.

Because kfree_rcu() has a fallback available (just using the old linked list), kfree_rcu() would work well given a way to tell the memory allocator to return NULL instead of acquiring a non-raw spinlock. Which is exactly what Ulad's recent patches are intended to do.

Since then, this thread has discussed various other approaches, including using existing combinations of GFP_ flags, converting the allocator's zone lock to a raw spinlock, and so on.

Does that help, or am I missing the point of your question?

Quoted 8 lines · Paul E. McKenney

Hence Ulad's work on kfree_rcu(). The approach is to allocate a page-sized array to hold all the pointers, then fill in the rest of these pointers on each subsequent kfree_rcu() call. These arrays of pointers also allows use of kfree_bulk() instead of kfree(), which can improve performance yet more. It is no big deal if kfree_rcu()'s allocation attempts fail occasionally because it can simply fall back to the old linked-list approach. And given that the various lockless caches in the memory allocator are almost never empty, in theory life is good.

Of course, it's always the damned reality which ruins the fun.

Quoted 8 lines

But in practice, mainline now has CONFIG_PROVE_RAW_LOCK_NESTING, and for good reason -- this Kconfig option makes it at least a little bit harder for mainline developers to mess up RT. But with CONFIG_PROVE_RAW_LOCK_NESTING=y and lockdep enabled, mainline will now sometimes complain if you invoke kfree_rcu() while holding a raw spinlock. This happens when kfree_rcu() needs to invoke the memory allocator and the memory allocator's caches are empty, thus resulting in the memory allocator attempting to acquire a non-raw spinlock.

Right.

Quoted 4 lines

Because kfree_rcu() has a fallback available (just using the old linked list), kfree_rcu() would work well given a way to tell the memory allocator to return NULL instead of acquiring a non-raw spinlock. Which is exactly what Ulad's recent patches are intended to do.

That much I understood, but I somehow failed to figure the why out despite the elaborate changelog. 2 weeks of 30+C seem to have cooked my brain :)

Quoted 5 lines

Since then, this thread has discussed various other approaches, including using existing combinations of GFP_ flags, converting the allocator's zone lock to a raw spinlock, and so on.

Does that help, or am I missing the point of your question?

Yes, that helps so far that I understand what the actual problem is. It does not really help to make me more happy. :)

That said, we can support atomic allocations on RT up to the point where zone->lock comes into play. We don't know yet exactly where the zone->lock induced damage happens. Presumably it's inside free_pcppages_bulk() - at least that's where I have faint bad memories from 15+ years ago. Aside of that I seriously doubt that it can be made work within a reasonable time frame.

But what makes me really unhappy is that my defense line against allocations from truly atomic contexts (from RT POV) which was enforced on RT gets a real big gap shot into it.

It becomes pretty hard to argue why atomic allocations via kmalloc() or kmem_cache_alloc() should be treated any different. Technically they can work similar to the page allocations up to the point where regular spinlocks come into play or the slab cache is exhausted. Where to draw the line?

It's also unclear for the page allocator case whether we can and should stick a limit on the number of pages and/or the pageorder.

Myself and others spent a considerable amount of time to kill off these kind of allocations from various interesting places including the guts of send IPI, the affinity setting path and others where people just slapped allocations into them because the stack checker warned or because they happened to copy the code from some other place.

RT was pretty much a quick crap detector whenever new incarnations of this got added and to some extent continuous education about these issues made them less prominent over the years. Using atomic allocations should always have a real good rationale, not only in the cases where they collide with RT.

I can understand your rationale and what you are trying to solve. So, if we can actually have a distinct GFP variant:

GFP_I_ABSOLUTELY_HAVE_TO_DO_THAT_AND_I_KNOW_IT_CAN_FAIL_EARLY

which is easy to grep for then having the page allocator go down to the point where zone lock gets involved is not the end of the world for RT in theory - unless that damned reality tells otherwise. :)

The page allocator allocations should also have a limit on the number of pages and eventually also page order (need to stare at the code or let Michal educate me that the order does not matter).

To make it consistent the same GFP_ variant should allow the slab allocator go to the point where the slab cache is exhausted.

Having a distinct and clearly defined GFP_ variant is really key to chase down offenders and to make reviewers double check upfront why this is absolutely required.

Quoted 3 lines · Thomas Gleixner

That much I understood, but I somehow failed to figure the why out despite the elaborate changelog. 2 weeks of 30+C seem to have cooked my brain :)

Ouch!!! And what on earth is Germany doing being that warm???

Quoted 3 lines

But what makes me really unhappy is that my defense line against allocations from truly atomic contexts (from RT POV) which was enforced on RT gets a real big gap shot into it.

Understood, and agreed: We do need to keep the RT degradation in check.

Quoted 8 lines

I can understand your rationale and what you are trying to solve. So, if we can actually have a distinct GFP variant:

GFP_I_ABSOLUTELY_HAVE_TO_DO_THAT_AND_I_KNOW_IT_CAN_FAIL_EARLY

which is easy to grep for then having the page allocator go down to the point where zone lock gets involved is not the end of the world for RT in theory - unless that damned reality tells otherwise. :)

I have no objection to an otherwise objectionable name in this particular case. After all, we now have 100 characters per line, right? ;-)

Quoted 2 lines

To make it consistent the same GFP_ variant should allow the slab allocator go to the point where the slab cache is exhausted.

Why not wait until someone has an extremely good reason for needing this functionality from the slab allocators? After all, leaving out the slab allocators would provide a more robust defense line. Yes, consistent APIs are very good things as a general rule, but maybe this situation is one of the exceptions to that rule.

Quoted 3 lines

Having a distinct and clearly defined GFP_ variant is really key to chase down offenders and to make reviewers double check upfront why this is absolutely required.

Checks for that GFP_ variant could be added to automation, though reality might eventually prove that to be a mixed blessing.

Paul,

Quoted 5 lines · Paul E. McKenney

Quoted 3 lines · Thomas Gleixner

That much I understood, but I somehow failed to figure the why out despite the elaborate changelog. 2 weeks of 30+C seem to have cooked my brain :)

Quoted 1 line

Ouch!!! And what on earth is Germany doing being that warm???

The hot air exhaustion of politicians, managers and conspiracy mythomaniacs seens to have contributed extensivly to global warming lately.

Quoted 6 lines

Quoted 3 lines

But what makes me really unhappy is that my defense line against allocations from truly atomic contexts (from RT POV) which was enforced on RT gets a real big gap shot into it.

Understood, and agreed: We do need to keep the RT degradation in check.

Not only that. It's bad practice in general to do memory allocations from such contexts if not absolutely necessary and the majority of cases which we cleaned up over time were just from the "works for me and why should I care and start to think" departement.

Quoted 9 lines

Quoted 8 lines

I can understand your rationale and what you are trying to solve. So, if we can actually have a distinct GFP variant:

GFP_I_ABSOLUTELY_HAVE_TO_DO_THAT_AND_I_KNOW_IT_CAN_FAIL_EARLY

which is easy to grep for then having the page allocator go down to the point where zone lock gets involved is not the end of the world for RT in theory - unless that damned reality tells otherwise. :)

Quoted 2 lines

I have no objection to an otherwise objectionable name in this particular case. After all, we now have 100 characters per line, right? ;-)

Hehe. I can live with the proposed NO_LOCK name or anything distinct which the mm people can agree on.

Quoted 8 lines

Quoted 2 lines

To make it consistent the same GFP_ variant should allow the slab allocator go to the point where the slab cache is exhausted.

Why not wait until someone has an extremely good reason for needing this functionality from the slab allocators? After all, leaving out the slab allocators would provide a more robust defense line. Yes, consistent APIs are very good things as a general rule, but maybe this situation is one of the exceptions to that rule.

Fair enough.

Quoted 4 lines

Quoted 3 lines

Having a distinct and clearly defined GFP_ variant is really key to chase down offenders and to make reviewers double check upfront why this is absolutely required.

Quoted 2 lines

Checks for that GFP_ variant could be added to automation, though reality might eventually prove that to be a mixed blessing.

Did you really have to remind me and destroy my illusions before I was able to marvel at them?

I can understand your rationale and what you are trying to solve. So, if we can actually have a distinct GFP variant:

GFP_I_ABSOLUTELY_HAVE_TO_DO_THAT_AND_I_KNOW_IT_CAN_FAIL_EARLY

which is easy to grep for then having the page allocator go down to the point where zone lock gets involved is not the end of the world for RT in theory - unless that damned reality tells otherwise. :)

The page allocator allocations should also have a limit on the number of pages and eventually also page order (need to stare at the code or let Michal educate me that the order does not matter).

To make it consistent the same GFP_ variant should allow the slab allocator go to the point where the slab cache is exhausted.

Having a distinct and clearly defined GFP_ variant is really key to chase down offenders and to make reviewers double check upfront why this is absolutely required.

On Wed 12-08-20 02:13:25, Thomas Gleixner wrote:
[...]

Quoted 4 lines

I can understand your rationale and what you are trying to solve. So, if we can actually have a distinct GFP variant:

GFP_I_ABSOLUTELY_HAVE_TO_DO_THAT_AND_I_KNOW_IT_CAN_FAIL_EARLY

Even if we cannot make the zone->lock raw I would prefer to not introduce a new gfp flag. Well we can do an alias for easier grepping
#define GFP_RT_SAFE 0

that would imply nowait semantic and would exclude waking up kswapd as well. If we can make wake up safe under RT then the alias would reflect that without any code changes.

The second, and the more important part, would be to bail out anytime the page allocator is to take a lock which is not allowed in the current RT context. Something like

include/linux/gfp.h +4 -0

index 67a0774e080b..3ef3ac82d110 100644
@@ -237,6 +237,9 @@ struct vm_area_struct;
  * that subsystems start with one of these combinations and then set/clear
  * %__GFP_FOO flags as necessary.
  *
+ * %GFP_RT_SAFE users can not sleep and they are running under RT atomic context
+ * e.g. under raw_spin_lock. Failure of an allocation is to be expected.
+ *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
  * watermark is applied to allow access to "atomic reserves"
  *

@@ -293,6 +296,7 @@ struct vm_area_struct;
  * version does not attempt reclaim/compaction at all and is by default used
  * in page fault path, while the non-light is used by khugepaged.
  */
+#define GFP_RT_SAFE	0
 #define GFP_ATOMIC	(__GFP_HIGH|__GFP_ATOMIC|__GFP_KSWAPD_RECLAIM)
 #define GFP_KERNEL	(__GFP_RECLAIM | __GFP_IO | __GFP_FS)
 #define GFP_KERNEL_ACCOUNT (GFP_KERNEL | __GFP_ACCOUNT)

mm/page_alloc.c +20 -0

index e028b87ce294..268ae872cc2a 100644
@@ -2824,6 +2824,13 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
 {
 	int i, alloced = 0;
 
+	/*
+	 * Hard atomic contexts are not supported by the allocator for
+	 * anything but pcp requests
+	 */
+	if (!preemtable())
+		return 0;
+
 	spin_lock(&zone->lock);
 	for (i = 0; i < count; ++i) {
 		struct page *page = __rmqueue(zone, order, migratetype,

@@ -3371,6 +3378,13 @@ struct page *rmqueue(struct zone *preferred_zone,
 		goto out;
 	}
 
+	/*
+	 * Hard atomic contexts are not supported by the allocator for high
+	 * order requests
+	 */
+	if (WARN_ON_ONCE(!preemtable()))
+		reurn NULL;
+
 	/*
 	 * We most definitely don't want callers attempting to
 	 * allocate greater than order-1 page units with __GFP_NOFAIL.

@@ -4523,6 +4537,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 				(__GFP_ATOMIC|__GFP_DIRECT_RECLAIM)))
 		gfp_mask &= ~__GFP_ATOMIC;
 
+	/* Hard atomic contexts support is very limited to the fast path */
+	if (!preemtable()) {
+		WARN_ON_ONCE(gfp_mask != GFP_RT_SAFE);
+		return NULL;
+	}
+
 retry_cpuset:
 	compaction_retries = 0;
 	no_progress_loops = 0;

What do you think?

Quoted 7 lines

which is easy to grep for then having the page allocator go down to the point where zone lock gets involved is not the end of the world for RT in theory - unless that damned reality tells otherwise. :)

The page allocator allocations should also have a limit on the number of pages and eventually also page order (need to stare at the code or let Michal educate me that the order does not matter).

In practice anything but order 0 is out of question because we need zone->lock for that currently. Maybe we can introduce pcp lists for higher orders in the future - I have a vague recollection Mel was playing with that some time ago.

Quoted 3 lines

To make it consistent the same GFP_ variant should allow the slab allocator go to the point where the slab cache is exhausted.

Quoted 3 lines

Having a distinct and clearly defined GFP_ variant is really key to chase down offenders and to make reviewers double check upfront why this is absolutely required.

Having a high level and recognizable gfp mask is OK but I would really like not to introduce a dedicated flag. The page allocator should be able to recognize the context which cannot be handled.

Sorry for jumping in. We can rely on preemptable() for sure, if CONFIG_PREEMPT_RT is enabled, something like below:

if (IS_ENABLED_RT && preemptebale())

Also i have a question about pcp-lists. Can we introduce and use all allowed MIGRATE_PCPTYPES? If called with GFP_RT_SAFE? If not please elaborate.

On Thu 13-08-20 11:58:40, Uladzislau Rezki wrote:
[...]

Quoted 4 lines

Sorry for jumping in. We can rely on preemptable() for sure, if CONFIG_PREEMPT_RT is enabled, something like below:

if (IS_ENABLED_RT && preemptebale())

Sure. I thought this was an RT specific thing that would be noop otherwise.

Quoted 9 lines · Michal Hocko

On Thu 13-08-20 11:58:40, Uladzislau Rezki wrote:
[...]

Quoted 4 lines

Sorry for jumping in. We can rely on preemptable() for sure, if CONFIG_PREEMPT_RT is enabled, something like below:

if (IS_ENABLED_RT && preemptebale())

Sure. I thought this was an RT specific thing that would be noop otherwise.

Well, even if RT specific it would be still something returning either true or false unconditionally.

And guarding it with RT is not working either because then you are back to square one with the problem which triggered the discussion in the first place:

raw_spin_lock()
alloc()

    if (RT && !preemptible())  <- False because RT == false
    	goto bail;

    spin_lock(&zone->lock)  --> LOCKDEP complains

So either you convince Paul not to do that or you need to do something like I suggested in my other reply.

And guarding it with RT is not working either because then you are back to square one with the problem which triggered the discussion in the first place:

raw_spin_lock()
alloc()

    if (RT && !preemptible())  <- False because RT == false
    	goto bail;

    spin_lock(&zone->lock)  --> LOCKDEP complains

So either you convince Paul not to do that or you need to do something like I suggested in my other reply.

Quoted 6 lines · Thomas Gleixner

And guarding it with RT is not working either because then you are back to square one with the problem which triggered the discussion in the first place:

raw_spin_lock()
alloc()

Quoted 7 lines

    if (RT && !preemptible())  <- False because RT == false
    	goto bail;

    spin_lock(&zone->lock)  --> LOCKDEP complains

So either you convince Paul not to do that or you need to do something like I suggested in my other reply.

I'd like to throw in the possibility that we do something like:

  raw_spin_lock()
    alloc()
      if (!spin_trylock(&zone->lock))
        if (RT && !preemptible())
          goto bail;
        spin_lock(&zone->lock);

would that make us feel more comfortable about converting zone->lock to a raw spinlock?

Quoted 24 lines · Matthew Wilcox

Quoted 6 lines · Thomas Gleixner

And guarding it with RT is not working either because then you are back to square one with the problem which triggered the discussion in the first place:

raw_spin_lock()
alloc()

Quoted 7 lines

    if (RT && !preemptible())  <- False because RT == false
    	goto bail;

    spin_lock(&zone->lock)  --> LOCKDEP complains

So either you convince Paul not to do that or you need to do something like I suggested in my other reply.

I'd like to throw in the possibility that we do something like:

  raw_spin_lock()
    alloc()
      if (!spin_trylock(&zone->lock))
        if (RT && !preemptible())
          goto bail;
        spin_lock(&zone->lock);

Quoted 2 lines

would that make us feel more comfortable about converting zone->lock to a raw spinlock?

Even if that could cure that particular problem of allocations in deep atomic context, making zone->lock raw brings back the problem of zone->lock being held/contended for hundreds of microseconds with interrupts disabled which is causing RT tasks to miss their deadlines by big margins.

Quoted 10 lines · Michal Hocko

On Wed 12-08-20 02:13:25, Thomas Gleixner wrote:
[...]

Quoted 4 lines

I can understand your rationale and what you are trying to solve. So, if we can actually have a distinct GFP variant:

GFP_I_ABSOLUTELY_HAVE_TO_DO_THAT_AND_I_KNOW_IT_CAN_FAIL_EARLY

Even if we cannot make the zone->lock raw I would prefer to not introduce a new gfp flag. Well we can do an alias for easier grepping
#define GFP_RT_SAFE 0

Quoted 3 lines

that would imply nowait semantic and would exclude waking up kswapd as well. If we can make wake up safe under RT then the alias would reflect that without any code changes.

mm/page_alloc.c static int rmqueue_bulk(struct zone *zone, unsigned int order, · line 2831

Quoted 9 lines

The second, and the more important part, would be to bail out anytime the page allocator is to take a lock which is not allowed in the current RT context. Something like

mm/page_alloc.c +5 -0

+	/*
+	 * Hard atomic contexts are not supported by the allocator for
+	 * anything but pcp requests
+	 */
+	if (!preemtable())

Quoted 8 lines

Quoted 3 lines

The page allocator allocations should also have a limit on the number of pages and eventually also page order (need to stare at the code or let Michal educate me that the order does not matter).

In practice anything but order 0 is out of question because we need zone->lock for that currently. Maybe we can introduce pcp lists for higher orders in the future - I have a vague recollection Mel was playing with that some time ago.

Quoted 10 lines

Quoted 6 lines

To make it consistent the same GFP_ variant should allow the slab allocator go to the point where the slab cache is exhausted.

Having a distinct and clearly defined GFP_ variant is really key to chase down offenders and to make reviewers double check upfront why this is absolutely required.

Having a high level and recognizable gfp mask is OK but I would really like not to introduce a dedicated flag. The page allocator should be able to recognize the context which cannot be handled.

Sorry for jumping in. We can rely on preemptable() for sure, if CONFIG_PREEMPT_RT is enabled, something like below:

if (IS_ENABLED_RT && preemptebale())

Quoted 8 lines · Uladzislau Rezki

Quoted 7 lines · Michal Hocko

On Wed 12-08-20 02:13:25, Thomas Gleixner wrote:
[...]

Quoted 4 lines

I can understand your rationale and what you are trying to solve. So, if we can actually have a distinct GFP variant:

GFP_I_ABSOLUTELY_HAVE_TO_DO_THAT_AND_I_KNOW_IT_CAN_FAIL_EARLY

Quoted 3 lines

Quoted 3 lines

Even if we cannot make the zone->lock raw I would prefer to not introduce a new gfp flag. Well we can do an alias for easier grepping
#define GFP_RT_SAFE 0

Just using 0 is sneaky but yes, that's fine :)

Bikeshedding: GFP_RT_NOWAIT or such might be more obvious.

Quoted 3 lines

Quoted 3 lines

that would imply nowait semantic and would exclude waking up kswapd as well. If we can make wake up safe under RT then the alias would reflect that without any code changes.

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

Quoted 3 lines

Quoted 3 lines

The second, and the more important part, would be to bail out anytime the page allocator is to take a lock which is not allowed in the current RT context. Something like

Quoted 5 lines

mm/page_alloc.c static int rmqueue_bulk(struct zone *zone, unsigned int order, · line 2831

Quoted 5 lines

mm/page_alloc.c +5 -0

+	/*
+	 * Hard atomic contexts are not supported by the allocator for
+	 * anything but pcp requests
+	 */
+	if (!preemtable())

If you make that preemtible() it might even compile, but that still wont work because if CONFIG_PREEMPT_COUNT=n then preemptible() is always false.

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

That would make it trivial as we could simply switch it over to rcu_wait.

Quoted 7 lines

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 6 lines

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

PeterZ might have opinions about that too I suspect.

Really, if your primary lockless caches are empty then any allocation which comes from deep atomic context should simply always fail. Being stuck in an interrupt handler or even deeper for 200+ microseconds waiting for zone lock is just bonkers IMO.

Quoted 24 lines · Thomas Gleixner

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

That would make it trivial as we could simply switch it over to rcu_wait.

Quoted 7 lines

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 6 lines

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

Quoted 7 lines

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

Hmm. I am likely missing something really important here. We have two problems at hand:
1) RT will become broken as soon as this new RCU functionality which requires an allocation from inside of raw_spinlock hits the RT tree
2) lockdep splats which are telling us that early because of the raw_spinlock-> spin_lock dependency.

1) can be handled by handled by the bailing out whenever we have to use zone->lock inside the buddy allocator - essentially even more strict NOWAIT semantic than we have for RT tree - proposed (pseudo) patch is trying to describe that.

2) would become a false positive if 1) is in place, right? RT wouldn't do the illegal nesting and !RT would just work fine because GFP_RT_NOWAIT would be simply GFP_NOWAIT & ~__GFP_KSWAPD_RECLAIM. Why should we limit the functionality of the allocator for something that is not a real problem?

Quoted 6 lines

PeterZ might have opinions about that too I suspect.

Really, if your primary lockless caches are empty then any allocation which comes from deep atomic context should simply always fail. Being stuck in an interrupt handler or even deeper for 200+ microseconds waiting for zone lock is just bonkers IMO.

That would require changing NOWAIT/ATOMIC allocations semantic quite drastically for !RT kernels as well. I am not sure this is something we can do. Or maybe I am just missing your point.

Quoted 33 lines · Michal Hocko

Quoted 24 lines · Thomas Gleixner

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

That would make it trivial as we could simply switch it over to rcu_wait.

Quoted 7 lines

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 6 lines

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

Quoted 7 lines

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

Quoted 6 lines

Hmm. I am likely missing something really important here. We have two problems at hand:
1) RT will become broken as soon as this new RCU functionality which requires an allocation from inside of raw_spinlock hits the RT tree
2) lockdep splats which are telling us that early because of the raw_spinlock-> spin_lock dependency.

That is a reasonable high-level summary.

Quoted 4 lines

1) can be handled by handled by the bailing out whenever we have to use zone->lock inside the buddy allocator - essentially even more strict NOWAIT semantic than we have for RT tree - proposed (pseudo) patch is trying to describe that.

Unless I am missing something subtle, the problem with this approach is that in production-environment CONFIG_PREEMPT_NONE=y kernels, there is no way at runtime to distinguish between holding a spinlock on the one hand and holding a raw spinlock on the other. Therefore, without some sort of indication from the caller, this approach will not make CONFIG_PREEMPT_NONE=y users happy.

Quoted 3 lines

That would require changing NOWAIT/ATOMIC allocations semantic quite drastically for !RT kernels as well. I am not sure this is something we can do. Or maybe I am just missing your point.

Exactly, and avoiding changing this semantic for current users is precisely why we are proposing some sort of indication to be passed into the allocation request. In Uladzislau's patch, this was the __GFP_NO_LOCKS flag, but whatever works.

Quoted 48 lines · Paul E. McKenney

Quoted 33 lines · Michal Hocko

Quoted 24 lines · Thomas Gleixner

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

That would make it trivial as we could simply switch it over to rcu_wait.

Quoted 7 lines

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 6 lines

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

Quoted 7 lines

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

Quoted 6 lines

Hmm. I am likely missing something really important here. We have two problems at hand:
1) RT will become broken as soon as this new RCU functionality which requires an allocation from inside of raw_spinlock hits the RT tree
2) lockdep splats which are telling us that early because of the raw_spinlock-> spin_lock dependency.

That is a reasonable high-level summary.

Quoted 4 lines

1) can be handled by handled by the bailing out whenever we have to use zone->lock inside the buddy allocator - essentially even more strict NOWAIT semantic than we have for RT tree - proposed (pseudo) patch is trying to describe that.

Quoted 6 lines

Unless I am missing something subtle, the problem with this approach is that in production-environment CONFIG_PREEMPT_NONE=y kernels, there is no way at runtime to distinguish between holding a spinlock on the one hand and holding a raw spinlock on the other. Therefore, without some sort of indication from the caller, this approach will not make CONFIG_PREEMPT_NONE=y users happy.

If the whole bailout is guarded by CONFIG_PREEMPT_RT specific atomicity check then there is no functional problem - GFP_RT_SAFE would still be GFP_NOWAIT so functional wise the allocator will still do the right thing.

[...]

Quoted 8 lines

Quoted 3 lines

That would require changing NOWAIT/ATOMIC allocations semantic quite drastically for !RT kernels as well. I am not sure this is something we can do. Or maybe I am just missing your point.

Exactly, and avoiding changing this semantic for current users is precisely why we are proposing some sort of indication to be passed into the allocation request. In Uladzislau's patch, this was the __GFP_NO_LOCKS flag, but whatever works.

As I've tried to explain already, I would really hope we can do without any new gfp flags. We are running out of them and they tend to generate a lot of maintenance burden. There is a lot of abuse etc. We should also not expose such an implementation detail of the allocator to callers because that would make future changes even harder. The alias, on the othere hand already builds on top of existing NOWAIT semantic and it just helps the allocator to complain about a wrong usage while it doesn't expose any internals.

On Thu, Aug 13, 2020 at 05:54:12PM +0200, Michal Hocko wrote:

Quoted 4 lines

If the whole bailout is guarded by CONFIG_PREEMPT_RT specific atomicity check then there is no functional problem - GFP_RT_SAFE would still be GFP_NOWAIT so functional wise the allocator will still do the right thing.

Perhaps it was just me getting confused, early hour Pacific Time and whatever other excuses might apply. But I thought that you still had an objection to GFP_RT_SAFE based on changes in allocator semantics for other users.

Quoted 1 line · Paul E. McKenney

On Thu, Aug 13, 2020 at 05:54:12PM +0200, Michal Hocko wrote:

[...]

Quoted 9 lines

Quoted 4 lines

If the whole bailout is guarded by CONFIG_PREEMPT_RT specific atomicity check then there is no functional problem - GFP_RT_SAFE would still be GFP_NOWAIT so functional wise the allocator will still do the right thing.

Perhaps it was just me getting confused, early hour Pacific Time and whatever other excuses might apply. But I thought that you still had an objection to GFP_RT_SAFE based on changes in allocator semantics for other users.

There is still that problem with lockdep complaining about raw->regular spinlock on !PREEMPT_RT that would need to get resolved somehow. Thomas is not really keen on adding some lockdep annotation mechanism and unfortunatelly I do not have a different idea how to get rid of those.

Quoted 13 lines · Michal Hocko

Quoted 1 line · Paul E. McKenney

On Thu, Aug 13, 2020 at 05:54:12PM +0200, Michal Hocko wrote:

[...]

Quoted 9 lines

Quoted 4 lines

If the whole bailout is guarded by CONFIG_PREEMPT_RT specific atomicity check then there is no functional problem - GFP_RT_SAFE would still be GFP_NOWAIT so functional wise the allocator will still do the right thing.

Perhaps it was just me getting confused, early hour Pacific Time and whatever other excuses might apply. But I thought that you still had an objection to GFP_RT_SAFE based on changes in allocator semantics for other users.

Quoted 4 lines

There is still that problem with lockdep complaining about raw->regular spinlock on !PREEMPT_RT that would need to get resolved somehow. Thomas is not really keen on adding some lockdep annotation mechanism and unfortunatelly I do not have a different idea how to get rid of those.

OK. So the current situation requires a choice between these these alternatives, each of which has shortcomings that have been mentioned earlier in this thread:

1. Prohibit invoking allocators from raw atomic context, such
as when holding a raw spinlock.

2. Adding a GFP_ flag.

3. Reusing existing GFP_ flags/values/whatever to communicate
the raw-context information that was to be communicated by the new GFP_ flag.

4. Making lockdep forgive acquiring spinlocks while holding
raw spinlocks, but only in CONFIG_PREEMPT_NONE=y kernels.

Quoted 19 lines · Paul E. McKenney

Quoted 13 lines · Michal Hocko

Quoted 1 line · Paul E. McKenney

On Thu, Aug 13, 2020 at 05:54:12PM +0200, Michal Hocko wrote:

[...]

Quoted 9 lines

Quoted 4 lines

If the whole bailout is guarded by CONFIG_PREEMPT_RT specific atomicity check then there is no functional problem - GFP_RT_SAFE would still be GFP_NOWAIT so functional wise the allocator will still do the right thing.

Perhaps it was just me getting confused, early hour Pacific Time and whatever other excuses might apply. But I thought that you still had an objection to GFP_RT_SAFE based on changes in allocator semantics for other users.

Quoted 4 lines

There is still that problem with lockdep complaining about raw->regular spinlock on !PREEMPT_RT that would need to get resolved somehow. Thomas is not really keen on adding some lockdep annotation mechanism and unfortunatelly I do not have a different idea how to get rid of those.

Quoted 8 lines

OK. So the current situation requires a choice between these these alternatives, each of which has shortcomings that have been mentioned earlier in this thread:

1. Prohibit invoking allocators from raw atomic context, such
as when holding a raw spinlock.

2. Adding a GFP_ flag.

Which would implemente a completely new level atomic allocation for all preemption models

Quoted 4 lines

3. Reusing existing GFP_ flags/values/whatever to communicate
the raw-context information that was to be communicated by the new GFP_ flag.

this would have to be RT specific to not change the semantic for existing users. In other words make NOWAIT semantic working for RT atomic contexts.

Quoted 1 line

Quoted 2 lines

4. Making lockdep forgive acquiring spinlocks while holding
raw spinlocks, but only in CONFIG_PREEMPT_NONE=y kernels.

and this would have to go along with 3 to remove false positives on !RT.

OK. So the current situation requires a choice between these these alternatives, each of which has shortcomings that have been mentioned earlier in this thread:

1. Prohibit invoking allocators from raw atomic context, such
as when holding a raw spinlock.

2. Adding a GFP_ flag.

3. Reusing existing GFP_ flags/values/whatever to communicate
the raw-context information that was to be communicated by the new GFP_ flag.

4. Making lockdep forgive acquiring spinlocks while holding
raw spinlocks, but only in CONFIG_PREEMPT_NONE=y kernels.

Am I missing anything?

On Thu, Aug 13, 2020 at 09:29:04AM -0700, Paul E. McKenney wrote:

Quoted 8 lines

3. Reusing existing GFP_ flags/values/whatever to communicate
the raw-context information that was to be communicated by the new GFP_ flag.

4. Making lockdep forgive acquiring spinlocks while holding
raw spinlocks, but only in CONFIG_PREEMPT_NONE=y kernels.

Am I missing anything?

How would 4 solve anything?

Quoted 48 lines · Michal Hocko

Quoted 33 lines · Thomas Gleixner

Quoted 24 lines

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

That would make it trivial as we could simply switch it over to rcu_wait.

Quoted 7 lines

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 6 lines

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

Quoted 7 lines

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

Quoted 6 lines

Hmm. I am likely missing something really important here. We have two problems at hand:
1) RT will become broken as soon as this new RCU functionality which requires an allocation from inside of raw_spinlock hits the RT tree
2) lockdep splats which are telling us that early because of the raw_spinlock-> spin_lock dependency.

That is a reasonable high-level summary.

Quoted 4 lines

1) can be handled by handled by the bailing out whenever we have to use zone->lock inside the buddy allocator - essentially even more strict NOWAIT semantic than we have for RT tree - proposed (pseudo) patch is trying to describe that.

Quoted 6 lines

Unless I am missing something subtle, the problem with this approach is that in production-environment CONFIG_PREEMPT_NONE=y kernels, there is no way at runtime to distinguish between holding a spinlock on the one hand and holding a raw spinlock on the other. Therefore, without some sort of indication from the caller, this approach will not make CONFIG_PREEMPT_NONE=y users happy.

If the whole bailout is guarded by CONFIG_PREEMPT_RT specific atomicity check then there is no functional problem - GFP_RT_SAFE would still be GFP_NOWAIT so functional wise the allocator will still do the right thing.

[...]

Quoted 8 lines

Quoted 3 lines

That would require changing NOWAIT/ATOMIC allocations semantic quite drastically for !RT kernels as well. I am not sure this is something we can do. Or maybe I am just missing your point.

Exactly, and avoiding changing this semantic for current users is precisely why we are proposing some sort of indication to be passed into the allocation request. In Uladzislau's patch, this was the __GFP_NO_LOCKS flag, but whatever works.

As I've tried to explain already, I would really hope we can do without any new gfp flags. We are running out of them and they tend to generate a lot of maintenance burden. There is a lot of abuse etc. We should also not expose such an implementation detail of the allocator to callers because that would make future changes even harder. The alias, on the othere hand already builds on top of existing NOWAIT semantic and it just helps the allocator to complain about a wrong usage while it doesn't expose any internals.

Quoted 72 lines · Paul E. McKenney

Quoted 48 lines · Michal Hocko

Quoted 33 lines · Thomas Gleixner

Quoted 24 lines

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

That would make it trivial as we could simply switch it over to rcu_wait.

Quoted 7 lines

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 6 lines

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

Quoted 7 lines

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

Quoted 6 lines

Hmm. I am likely missing something really important here. We have two problems at hand:
1) RT will become broken as soon as this new RCU functionality which requires an allocation from inside of raw_spinlock hits the RT tree
2) lockdep splats which are telling us that early because of the raw_spinlock-> spin_lock dependency.

That is a reasonable high-level summary.

Quoted 4 lines

1) can be handled by handled by the bailing out whenever we have to use zone->lock inside the buddy allocator - essentially even more strict NOWAIT semantic than we have for RT tree - proposed (pseudo) patch is trying to describe that.

Quoted 6 lines

Unless I am missing something subtle, the problem with this approach is that in production-environment CONFIG_PREEMPT_NONE=y kernels, there is no way at runtime to distinguish between holding a spinlock on the one hand and holding a raw spinlock on the other. Therefore, without some sort of indication from the caller, this approach will not make CONFIG_PREEMPT_NONE=y users happy.

If the whole bailout is guarded by CONFIG_PREEMPT_RT specific atomicity check then there is no functional problem - GFP_RT_SAFE would still be GFP_NOWAIT so functional wise the allocator will still do the right thing.

[...]

Quoted 8 lines

Quoted 3 lines

That would require changing NOWAIT/ATOMIC allocations semantic quite drastically for !RT kernels as well. I am not sure this is something we can do. Or maybe I am just missing your point.

Exactly, and avoiding changing this semantic for current users is precisely why we are proposing some sort of indication to be passed into the allocation request. In Uladzislau's patch, this was the __GFP_NO_LOCKS flag, but whatever works.

Quoted 9 lines

As I've tried to explain already, I would really hope we can do without any new gfp flags. We are running out of them and they tend to generate a lot of maintenance burden. There is a lot of abuse etc. We should also not expose such an implementation detail of the allocator to callers because that would make future changes even harder. The alias, on the othere hand already builds on top of existing NOWAIT semantic and it just helps the allocator to complain about a wrong usage while it doesn't expose any internals.

I know that Matthew and me raised it. We do can handle it without introducing any flag. I mean just use 0 as argument to the page_alloc(gfp_flags = 0)

i.e. #define __GFP_NO_LOCKS 0

so it will be handled same way how it is done in the "mm: Add __GFP_NO_LOCKS flag" I can re-spin the RFC patch and send it out for better understanding.

Does it work for you, Michal? Or it is better just to drop the patch here?

Quoted 86 lines · Uladzislau Rezki

Quoted 72 lines · Paul E. McKenney

Quoted 48 lines · Michal Hocko

Quoted 33 lines · Thomas Gleixner

Quoted 24 lines

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

That would make it trivial as we could simply switch it over to rcu_wait.

Quoted 7 lines

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 6 lines

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

Quoted 7 lines

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

Quoted 6 lines

Hmm. I am likely missing something really important here. We have two problems at hand:
1) RT will become broken as soon as this new RCU functionality which requires an allocation from inside of raw_spinlock hits the RT tree
2) lockdep splats which are telling us that early because of the raw_spinlock-> spin_lock dependency.

That is a reasonable high-level summary.

Quoted 4 lines

1) can be handled by handled by the bailing out whenever we have to use zone->lock inside the buddy allocator - essentially even more strict NOWAIT semantic than we have for RT tree - proposed (pseudo) patch is trying to describe that.

Quoted 6 lines

Unless I am missing something subtle, the problem with this approach is that in production-environment CONFIG_PREEMPT_NONE=y kernels, there is no way at runtime to distinguish between holding a spinlock on the one hand and holding a raw spinlock on the other. Therefore, without some sort of indication from the caller, this approach will not make CONFIG_PREEMPT_NONE=y users happy.

If the whole bailout is guarded by CONFIG_PREEMPT_RT specific atomicity check then there is no functional problem - GFP_RT_SAFE would still be GFP_NOWAIT so functional wise the allocator will still do the right thing.

[...]

Quoted 8 lines

Quoted 3 lines

That would require changing NOWAIT/ATOMIC allocations semantic quite drastically for !RT kernels as well. I am not sure this is something we can do. Or maybe I am just missing your point.

Exactly, and avoiding changing this semantic for current users is precisely why we are proposing some sort of indication to be passed into the allocation request. In Uladzislau's patch, this was the __GFP_NO_LOCKS flag, but whatever works.

Quoted 9 lines

As I've tried to explain already, I would really hope we can do without any new gfp flags. We are running out of them and they tend to generate a lot of maintenance burden. There is a lot of abuse etc. We should also not expose such an implementation detail of the allocator to callers because that would make future changes even harder. The alias, on the othere hand already builds on top of existing NOWAIT semantic and it just helps the allocator to complain about a wrong usage while it doesn't expose any internals.

I know that Matthew and me raised it. We do can handle it without introducing any flag. I mean just use 0 as argument to the page_alloc(gfp_flags = 0)

i.e. #define __GFP_NO_LOCKS 0

Quoted 4 lines

so it will be handled same way how it is done in the "mm: Add __GFP_NO_LOCKS flag" I can re-spin the RFC patch and send it out for better understanding.

Does it work for you, Michal? Or it is better just to drop the patch here?

That would change the semantic for GFP_NOWAIT users who decided to drop __GFP_KSWAPD_RECLAIM or even use 0 gfp mask right away, right? The point

I am trying to make is that an alias is good for RT because it doesn't have any users (because there is no RT atomic user of the allocator) currently.

Quoted 11 lines · Thomas Gleixner

Quoted 2 lines · Michal Hocko

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

Hmm. I am likely missing something really important here. We have two problems at hand:
1) RT will become broken as soon as this new RCU functionality which requires an allocation from inside of raw_spinlock hits the RT tree
2) lockdep splats which are telling us that early because of the raw_spinlock-> spin_lock dependency.

1) can be handled by handled by the bailing out whenever we have to use zone->lock inside the buddy allocator - essentially even more strict NOWAIT semantic than we have for RT tree - proposed (pseudo) patch is trying to describe that.

2) would become a false positive if 1) is in place, right? RT wouldn't do the illegal nesting and !RT would just work fine because GFP_RT_NOWAIT would be simply GFP_NOWAIT & ~__GFP_KSWAPD_RECLAIM.

Why should we limit the functionality of the allocator for something that is not a real problem?

Quoted 4 lines

Really, if your primary lockless caches are empty then any allocation which comes from deep atomic context should simply always fail. Being stuck in an interrupt handler or even deeper for 200+ microseconds waiting for zone lock is just bonkers IMO.

That would require changing NOWAIT/ATOMIC allocations semantic quite drastically for !RT kernels as well. I am not sure this is something we can do. Or maybe I am just missing your point.

Quoted 1 line · Michal Hocko

On Thu 13-08-20 16:34:57, Thomas Gleixner wrote:

Quoted 8 lines

Quoted 4 lines

Really, if your primary lockless caches are empty then any allocation which comes from deep atomic context should simply always fail. Being stuck in an interrupt handler or even deeper for 200+ microseconds waiting for zone lock is just bonkers IMO.

That would require changing NOWAIT/ATOMIC allocations semantic quite drastically for !RT kernels as well. I am not sure this is something we can do. Or maybe I am just missing your point.

I really do not understand why you think that it affects everything.

Michal Hocko <[email protected]> writes:

Quoted 2 lines

Why should we limit the functionality of the allocator for something that is not a real problem?

We'd limit the allocator for exactly ONE new user which was aware of this problem _before_ the code hit mainline. And that ONE user is prepared to handle the fail.

Even if we could make this lockdep thing work that does not mean that it's a good thing to do.

Quite the contrary, you'd just encourage people to create more of those use cases for probably the completely wrong reasons. Putting a limitation into place upfront might makes them think farther than just slapping GFP_RT_ATOMIC in and be done with it. Let me dream :)

I've dealt with tons of patches in the last 15+ years where people just came up with 's/GFP_KERNEL/GFP_ATOMIC/ because tool complained' patches. The vast majority of them were bogus because the alloc() was simply at the wrong place.

Forcing people not to take the easy way out by making the infrastructure restrictive is way better than encouraging mindless hackery. We have enough of this (not restricted to memory allocations) all over the kernel already. No need for more.

Quoted 1 line

Michal Hocko <[email protected]> writes:

[...]

Quoted 6 lines

Quoted 2 lines

Why should we limit the functionality of the allocator for something that is not a real problem?

We'd limit the allocator for exactly ONE new user which was aware of this problem _before_ the code hit mainline. And that ONE user is prepared to handle the fail.

If we are to limit the functionality to this one particular user then I would consider a dedicated gfp flag a huge overkill. It would be much more easier to have a preallocated pool of pages and use those and completely avoid the core allocator. That would certainly only shift the complexity to the caller but if it is expected there would be only that single user then it would be probably better than opening a can of worms like allocator usable from raw spin locks.

Quoted 10 lines · Thomas Gleixner

Quoted 1 line

Michal Hocko <[email protected]> writes:

[...]

Quoted 6 lines

Quoted 2 lines

Why should we limit the functionality of the allocator for something that is not a real problem?

We'd limit the allocator for exactly ONE new user which was aware of this problem _before_ the code hit mainline. And that ONE user is prepared to handle the fail.

Quoted 8 lines

If we are to limit the functionality to this one particular user then I would consider a dedicated gfp flag a huge overkill. It would be much more easier to have a preallocated pool of pages and use those and completely avoid the core allocator. That would certainly only shift the complexity to the caller but if it is expected there would be only that single user then it would be probably better than opening a can of worms like allocator usable from raw spin locks.

Vlastimil raised same question earlier, i answered, but let me answer again:

It is hard to achieve because the logic does not stick to certain static test case, i.e. it depends on how heavily kfree_rcu(single/double) are used. Based on that, "how heavily" - number of pages are formed, until the drain/reclaimer thread frees them.

Preloading pages and keeping them for internal use, IMHO, seems not optimal from the point of resources wasting. It is better to have a fast mechanism to request a page and release it back for needs of others. As described about we do not know how much we will need.

Quoted 18 lines · Uladzislau Rezki

Quoted 18 lines · Thomas Gleixner

Quoted 1 line

Michal Hocko <[email protected]> writes:

[...]

Quoted 6 lines

Quoted 2 lines

Why should we limit the functionality of the allocator for something that is not a real problem?

We'd limit the allocator for exactly ONE new user which was aware of this problem _before_ the code hit mainline. And that ONE user is prepared to handle the fail.

If we are to limit the functionality to this one particular user then I would consider a dedicated gfp flag a huge overkill. It would be much more easier to have a preallocated pool of pages and use those and completely avoid the core allocator. That would certainly only shift the complexity to the caller but if it is expected there would be only that single user then it would be probably better than opening a can of worms like allocator usable from raw spin locks.

Quoted 6 lines

Vlastimil raised same question earlier, i answered, but let me answer again:

It is hard to achieve because the logic does not stick to certain static test case, i.e. it depends on how heavily kfree_rcu(single/double) are used. Based on that, "how heavily" - number of pages are formed, until the drain/reclaimer thread frees them.

How many pages are talking about - ball park? 100s, 1000s?

Quoted 4 lines

Preloading pages and keeping them for internal use, IMHO, seems not optimal from the point of resources wasting. It is better to have a fast mechanism to request a page and release it back for needs of others. As described about we do not know how much we will need.

It would be wasted memory but if we are talking about relatively small number of pages. Note that there are not that many pages on the allocator's pcp list anyway.

Quoted 19 lines · Michal Hocko

Quoted 18 lines · Uladzislau Rezki

Quoted 18 lines · Thomas Gleixner

Quoted 1 line

Michal Hocko <[email protected]> writes:

[...]

Quoted 6 lines

Quoted 2 lines

Why should we limit the functionality of the allocator for something that is not a real problem?

We'd limit the allocator for exactly ONE new user which was aware of this problem _before_ the code hit mainline. And that ONE user is prepared to handle the fail.

If we are to limit the functionality to this one particular user then I would consider a dedicated gfp flag a huge overkill. It would be much more easier to have a preallocated pool of pages and use those and completely avoid the core allocator. That would certainly only shift the complexity to the caller but if it is expected there would be only that single user then it would be probably better than opening a can of worms like allocator usable from raw spin locks.

Quoted 8 lines

Quoted 6 lines

Vlastimil raised same question earlier, i answered, but let me answer again:

It is hard to achieve because the logic does not stick to certain static test case, i.e. it depends on how heavily kfree_rcu(single/double) are used. Based on that, "how heavily" - number of pages are formed, until the drain/reclaimer thread frees them.

How many pages are talking about - ball park? 100s, 1000s?

Under normal operation, a couple of pages per CPU, which would make preallocation entirely reasonable. Except that if someone does something that floods RCU callbacks (close(open) in a tight userspace loop, for but one example), then 2000 per CPU might not be enough, which on a 64-CPU system comes to about 500MB. This is beyond excessive for preallocation on the systems I am familiar with.

And the flooding case is where you most want the reclamation to be efficient, and thus where you want the pages.

This of course raises the question of how much memory the lockless caches contain, but fortunately these RCU callback flooding scenarios also involve process-context allocation of the memory that is being passed to kfree_rcu(). That allocation should keep the lockless caches from going empty in the common case, correct?

Quoted 36 lines · Paul E. McKenney

Quoted 19 lines · Michal Hocko

Quoted 18 lines · Uladzislau Rezki

Quoted 18 lines · Thomas Gleixner

Quoted 1 line

Michal Hocko <[email protected]> writes:

[...]

Quoted 6 lines

Quoted 2 lines

Why should we limit the functionality of the allocator for something that is not a real problem?

We'd limit the allocator for exactly ONE new user which was aware of this problem _before_ the code hit mainline. And that ONE user is prepared to handle the fail.

If we are to limit the functionality to this one particular user then I would consider a dedicated gfp flag a huge overkill. It would be much more easier to have a preallocated pool of pages and use those and completely avoid the core allocator. That would certainly only shift the complexity to the caller but if it is expected there would be only that single user then it would be probably better than opening a can of worms like allocator usable from raw spin locks.

Quoted 8 lines

Quoted 6 lines

Vlastimil raised same question earlier, i answered, but let me answer again:

It is hard to achieve because the logic does not stick to certain static test case, i.e. it depends on how heavily kfree_rcu(single/double) are used. Based on that, "how heavily" - number of pages are formed, until the drain/reclaimer thread frees them.

How many pages are talking about - ball park? 100s, 1000s?

Under normal operation, a couple of pages per CPU, which would make preallocation entirely reasonable. Except that if someone does something that floods RCU callbacks (close(open) in a tight userspace loop, for but one example), then 2000 per CPU might not be enough, which on a 64-CPU system comes to about 500MB. This is beyond excessive for preallocation on the systems I am familiar with.

Quoted 2 lines

And the flooding case is where you most want the reclamation to be efficient, and thus where you want the pages.

I am not sure the page allocator would help you with this scenario unless you are on very large machines. Pagesets scale with the available memory and percpu_pagelist_fraction sysctl (have a look at pageset_set_high_and_batch). It is roughly 1000th of the zone size for each zone. You can check that in /proc/vmstat (my 8G machine)

Node 0, zone DMA
Not interesting at all
Node 0, zone DMA32
pagesets

    cpu: 0
              count: 242
              high:  378
              batch: 63
    cpu: 1
              count: 355
              high:  378
              batch: 63
    cpu: 2
              count: 359
              high:  378
              batch: 63
    cpu: 3
              count: 366
              high:  378
              batch: 63
Node 0, zone   Normal
  pagesets
    cpu: 0
              count: 359
              high:  378
              batch: 63
    cpu: 1
              count: 241
              high:  378
              batch: 63
    cpu: 2
              count: 297
              high:  378
              batch: 63
    cpu: 3
              count: 227
              high:  378
              batch: 63

Besides that do you need to be per-cpu? Having 1000 pages available and managed under your raw spinlock should be good enough already no?

Quoted 5 lines

This of course raises the question of how much memory the lockless caches contain, but fortunately these RCU callback flooding scenarios also involve process-context allocation of the memory that is being passed to kfree_rcu(). That allocation should keep the lockless caches from going empty in the common case, correct?

Yes, those are refilled both on the allocation/free paths. But you cannot really rely on that to happen early enough.

Do you happen to have any numbers that would show the typical usage and how often the slow path has to be taken becase pcp lists are depleted? In other words even if we provide a functionality to give completely lockless way to allocate memory how useful that is?

Quoted 16 lines · Michal Hocko

Quoted 16 lines

Quoted 8 lines

Quoted 6 lines

Vlastimil raised same question earlier, i answered, but let me answer again:

It is hard to achieve because the logic does not stick to certain static test case, i.e. it depends on how heavily kfree_rcu(single/double) are used. Based on that, "how heavily" - number of pages are formed, until the drain/reclaimer thread frees them.

How many pages are talking about - ball park? 100s, 1000s?

Under normal operation, a couple of pages per CPU, which would make preallocation entirely reasonable. Except that if someone does something that floods RCU callbacks (close(open) in a tight userspace loop, for but one example), then 2000 per CPU might not be enough, which on a 64-CPU system comes to about 500MB. This is beyond excessive for preallocation on the systems I am familiar with.

Quoted 2 lines

Quoted 2 lines

And the flooding case is where you most want the reclamation to be efficient, and thus where you want the pages.

Paul,

Quoted 17 lines · Paul E. McKenney

Quoted 16 lines · Michal Hocko

Quoted 16 lines

Quoted 8 lines

Quoted 6 lines

Vlastimil raised same question earlier, i answered, but let me answer again:

It is hard to achieve because the logic does not stick to certain static test case, i.e. it depends on how heavily kfree_rcu(single/double) are used. Based on that, "how heavily" - number of pages are formed, until the drain/reclaimer thread frees them.

How many pages are talking about - ball park? 100s, 1000s?

Under normal operation, a couple of pages per CPU, which would make preallocation entirely reasonable. Except that if someone does something that floods RCU callbacks (close(open) in a tight userspace loop, for but one example), then 2000 per CPU might not be enough, which on a 64-CPU system comes to about 500MB. This is beyond excessive for preallocation on the systems I am familiar with.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

And the flooding case is where you most want the reclamation to be efficient, and thus where you want the pages.

As we now established that taking zone lock is impossible at all independent of raw/non-raw ordering and independent of RT/PREEMPT configs, can we just take a step back and look at the problem from scratch again?

As a matter of fact I assume^Wdeclare that removing struct rcu_head which provides a fallback is not an option at all. I know that you want to, but it wont work ever. Dream on, but as we agreed on recently there is this thing called reality which ruins everything.

For normal operations a couple of pages which can be preallocated are enough. What you are concerned of is the case where you run out of pointer storage space.

There are two reasons why that can happen:

1) RCU call flooding
2) RCU not being able to run and mop up the backlog

#1 is observable by looking at the remaining storage space and the RCU
call frequency

#2 is uninteresting because it's caused by RCU being stalled / delayed
e.g. by a runaway of some sorts or a plain RCU usage bug.

Allocating more memory in that case does not solve or improve anything.

So the interesting case is #1. Which means we need to look at the potential sources of the flooding:

1) User space via syscalls, e.g. open/close
2) Kernel thread
3) Softirq
4) Device interrupt
5) System interrupts, deep atomic context, NMI ...

#1 trivial fix is to force switching to an high prio thread or a soft
interrupt which does the allocation

#2 Similar to #1 unless that thread loops with interrupts, softirqs or
preemption disabled. If that's the case then running out of RCU storage space is the least of your worries.

#3 Similar to #2. The obvious candidates (e.g. NET) for monopolizing a
CPU have loop limits in place already. If there is a bug which fails to care about the limit, why would RCU care and allocate more memory?

#4 Similar to #3. If the interrupt handler loops forever or if the
interrupt is a runaway which prevents task/softirq processing then RCU free performance is the least of your worries.

#5 Clearly a bug and making RCU accomodate for that is beyond silly.

So if call_rcu() detects that the remaining storage space for pointers goes below the critical point or if it observes high frequency calls then it simply should force a soft interrupt which does the allocation.

Allocating from softirq context obviously without holding the raw lock which is used inside call_rcu() is safe on all configurations.

If call_rcu() is forced to use the fallback for a few calls until this happens then that's not the end of the world. It is not going to be a problem ever for the most obvious issue #1, user space madness, because that case cannot delay the softirq processing unless there is a kernel bug which makes again RCU free performance irrelevant.

So this will cure the problem for the most interesting case #1 and handle all sane variants of the other possible flooding sources.

The other insane reasons do not justify any attempt to increase RCU performance at all.

Watching the remaining storage space is good enough IMO. It clearly covers #1 and for all others the occasional fallback wont hurt. If it really matters for any case > #1 then doing a frequency based prediction is a straight forward optimization.

As usual I might be missing something, but as this is the second day with reasonable temperatures here that would be caused by my ignorance and not be excusable by brain usage outside of specified temperature range.

Quoted 4 lines · Thomas Gleixner

As a matter of fact I assume^Wdeclare that removing struct rcu_head which provides a fallback is not an option at all. I know that you want to, but it wont work ever. Dream on, but as we agreed on recently there is this thing called reality which ruins everything.

For call_rcu(), agreed. For kfree_rcu(), we know what the callback is going to do, plus single-argument kfree_rcu() can only be invoked from sleepable context. (If you want to kfree_rcu() from non-sleepable context, that will cost you an rcu_head in the data structure being freed.)

So if the single-argument kfree_rcu() case gets hit with a memory-allocation failure, it can fall back to waiting for a grace period and doing the free. Of course, grace-period waits have horrible latency, but under OOM life is hard. If this becomes a problem in non-OOM situations due to the lockless caches becoming empty, we will have to allocate memory if needed before acquiring the lock with the usual backout logic. Doing that means that we can let the allocator acquire locks and maybe even do a little bit of blocking, so that the inline grace-period-wait would only happen if the system was well and truly OOMed.

Quoted 3 lines

For normal operations a couple of pages which can be preallocated are enough. What you are concerned of is the case where you run out of pointer storage space.

Agreed.

Quoted 11 lines

There are two reasons why that can happen:

1) RCU call flooding
2) RCU not being able to run and mop up the backlog

#1 is observable by looking at the remaining storage space and the RCU
call frequency

#2 is uninteresting because it's caused by RCU being stalled / delayed
e.g. by a runaway of some sorts or a plain RCU usage bug.

Quoted 1 line

Allocating more memory in that case does not solve or improve anything.

Yes, #2 is instead RCU CPU stall warning territory.

If this becomes a problem, one approach is to skip the page-of-pointers allocation if the grace period is more than (say) one second old. If the grace period never completes, OOM is unavoidable, but this is a way of putting it off for a bit.

Quoted 26 lines

So the interesting case is #1. Which means we need to look at the potential sources of the flooding:

1) User space via syscalls, e.g. open/close
2) Kernel thread
3) Softirq
4) Device interrupt
5) System interrupts, deep atomic context, NMI ...

#1 trivial fix is to force switching to an high prio thread or a soft
interrupt which does the allocation

#2 Similar to #1 unless that thread loops with interrupts, softirqs or
preemption disabled. If that's the case then running out of RCU storage space is the least of your worries.

#3 Similar to #2. The obvious candidates (e.g. NET) for monopolizing a
CPU have loop limits in place already. If there is a bug which fails to care about the limit, why would RCU care and allocate more memory?

#4 Similar to #3. If the interrupt handler loops forever or if the
interrupt is a runaway which prevents task/softirq processing then RCU free performance is the least of your worries.

#5 Clearly a bug and making RCU accomodate for that is beyond silly.

Quoted 3 lines

So if call_rcu() detects that the remaining storage space for pointers goes below the critical point or if it observes high frequency calls then it simply should force a soft interrupt which does the allocation.

Unless call_rcu() has been invoked with scheduler locks held. But eventually call_rcu() should be invoked with interrupts enabled, and at that point it would be safe to raise_softirq(), wake_up(), or whatever.

Paul,

Quoted 11 lines · Paul E. McKenney

Quoted 4 lines · Thomas Gleixner

As a matter of fact I assume^Wdeclare that removing struct rcu_head which provides a fallback is not an option at all. I know that you want to, but it wont work ever. Dream on, but as we agreed on recently there is this thing called reality which ruins everything.

For call_rcu(), agreed. For kfree_rcu(), we know what the callback is going to do, plus single-argument kfree_rcu() can only be invoked from sleepable context. (If you want to kfree_rcu() from non-sleepable context, that will cost you an rcu_head in the data structure being freed.)

kfree_rcu() as of today is just a conveniance wrapper around call_rcu(obj, rcu) which can be called from any context and it still takes TWO arguments.

Icepack?

So if you come up with a new kfree_rcu_magic(void *obj) single argument variant which can only be called from sleepable contexts then this does not require any of the raw lock vs. non raw hacks at all because you can simply allocate without holding the raw lock in the rare case that you run out of storage space. With four 4k pages preallocated per CPU that's every 2048 invocations per CPU on 64bit.

So if you run into that situation then you drop the lock and then it's racy because you might be preempted or migrated after dropping the lock and you might have done a useless allocation, but that does not justify having a special allocator just for that? You have an extra page, so what?

To prevent subsequent callers to add to the allocation race you simply can let them wait on the first allocating attempt to finish That avoids more pointless allocations and as a side effect prevents all of them to create more pressure by continuing their open/close loop naturally without extra work.

Quoted 10 lines

So if the single-argument kfree_rcu() case gets hit with a memory-allocation failure, it can fall back to waiting for a grace period and doing the free. Of course, grace-period waits have horrible latency, but under OOM life is hard. If this becomes a problem in non-OOM situations due to the lockless caches becoming empty, we will have to allocate memory if needed before acquiring the lock with the usual backout logic. Doing that means that we can let the allocator acquire locks and maybe even do a little bit of blocking, so that the inline grace-period-wait would only happen if the system was well and truly OOMed.

No. It dropped the rcu internal lock and does a regular GFP_KENRNEL allocation which waits for the page to become available. Which is a good thing in the open/close scenario because it throttles the offender.

Quoted 21 lines

Quoted 3 lines

For normal operations a couple of pages which can be preallocated are enough. What you are concerned of is the case where you run out of pointer storage space.

Agreed.

Quoted 5 lines

There are two reasons why that can happen:

1) RCU call flooding
2) RCU not being able to run and mop up the backlog

Quoted 7 lines

#1 is observable by looking at the remaining storage space and the RCU
call frequency

#2 is uninteresting because it's caused by RCU being stalled / delayed
e.g. by a runaway of some sorts or a plain RCU usage bug.

Allocating more memory in that case does not solve or improve anything.

Yes, #2 is instead RCU CPU stall warning territory.

Quoted 4 lines

If this becomes a problem, one approach is to skip the page-of-pointers allocation if the grace period is more than (say) one second old. If the grace period never completes, OOM is unavoidable, but this is a way of putting it off for a bit.

Don't even think about optimizing your new thing for #2. It's a pointless exercise. If the task which runs into the 'can't allocate' case then is sleeps and waits. End of story.

Quoted 26 lines

Quoted 26 lines

So the interesting case is #1. Which means we need to look at the potential sources of the flooding:

1) User space via syscalls, e.g. open/close
2) Kernel thread
3) Softirq
4) Device interrupt
5) System interrupts, deep atomic context, NMI ...

#1 trivial fix is to force switching to an high prio thread or a soft
interrupt which does the allocation

#2 Similar to #1 unless that thread loops with interrupts, softirqs or
preemption disabled. If that's the case then running out of RCU storage space is the least of your worries.

#3 Similar to #2. The obvious candidates (e.g. NET) for monopolizing a
CPU have loop limits in place already. If there is a bug which fails to care about the limit, why would RCU care and allocate more memory?

#4 Similar to #3. If the interrupt handler loops forever or if the
interrupt is a runaway which prevents task/softirq processing then RCU free performance is the least of your worries.

#5 Clearly a bug and making RCU accomodate for that is beyond silly.

Quoted 8 lines

Quoted 3 lines

So if call_rcu() detects that the remaining storage space for pointers goes below the critical point or if it observes high frequency calls then it simply should force a soft interrupt which does the allocation.

Unless call_rcu() has been invoked with scheduler locks held. But eventually call_rcu() should be invoked with interrupts enabled, and at that point it would be safe to raise_softirq(), wake_up(), or whatever.

If this atomic context corner case is hit within a problematic context then we talk about the RCU of today and not about the future single argument thing. And that oldschool RCU has a fallback. We are talking about pressure corner cases and you really want to squeeze out the last cache miss? What for? If there is pressure then these cache misses are irrelevant.

As a matter of fact I assume^Wdeclare that removing struct rcu_head which provides a fallback is not an option at all. I know that you want to, but it wont work ever. Dream on, but as we agreed on recently there is this thing called reality which ruins everything.

Quoted 5 lines · Thomas Gleixner

As a matter of fact I assume^Wdeclare that removing struct rcu_head which provides a fallback is not an option at all. I know that you want to, but it wont work ever. Dream on, but as we agreed on recently there is this thing called reality which ruins everything.

It never was going to work, freeing memory can never hard rely on the success of allocating memory.

#1 trivial fix is to force switching to an high prio thread or a soft
interrupt which does the allocation

Quoted 2 lines · Thomas Gleixner

#1 trivial fix is to force switching to an high prio thread or a soft
interrupt which does the allocation

Yeah, push the alocation out to another context. I did consider it, but why bother?

Also, raising a softirq can't be done from every context, that's a whole new problem. You can do irq_work I suppose, but not all architectures support the self-IPI yet.

All in all, it's just more complexity than the fairly trivial __alloc_page_lockless().

Whichever way around, we can't rely on the allocation.

Quoted 15 lines · Peter Zijlstra

Quoted 2 lines · Thomas Gleixner

#1 trivial fix is to force switching to an high prio thread or a soft
interrupt which does the allocation

Yeah, push the alocation out to another context. I did consider it, but why bother?

Also, raising a softirq can't be done from every context, that's a whole new problem. You can do irq_work I suppose, but not all architectures support the self-IPI yet.

All in all, it's just more complexity than the fairly trivial __alloc_page_lockless().

Quoted 1 line

Whichever way around, we can't rely on the allocation.

One way to enforce that would be to put something like this at the beginning of the __alloc_page_lockless() function:

	if (IS_ENABLED(CONFIG_PROVE_LOCKING) && (prandom_u32() & 0xffff))
		return NULL;

I am sure that there is a better choice than CONFIG_PROVE_LOCKING. But whatever the choice, there is nothing quite like the occasional allocation failure during testing to convince people that such failure really can happen.

Thanx, Paul

For normal operations a couple of pages which can be preallocated are enough. What you are concerned of is the case where you run out of pointer storage space.

There are two reasons why that can happen:

1) RCU call flooding
2) RCU not being able to run and mop up the backlog

#1 is observable by looking at the remaining storage space and the RCU
call frequency

#2 is uninteresting because it's caused by RCU being stalled / delayed
e.g. by a runaway of some sorts or a plain RCU usage bug.

Allocating more memory in that case does not solve or improve anything.

So the interesting case is #1. Which means we need to look at the potential sources of the flooding:

1) User space via syscalls, e.g. open/close
2) Kernel thread
3) Softirq
4) Device interrupt
5) System interrupts, deep atomic context, NMI ...

#1 trivial fix is to force switching to an high prio thread or a soft
interrupt which does the allocation

#2 Similar to #1 unless that thread loops with interrupts, softirqs or
preemption disabled. If that's the case then running out of RCU storage space is the least of your worries.

#3 Similar to #2. The obvious candidates (e.g. NET) for monopolizing a
CPU have loop limits in place already. If there is a bug which fails to care about the limit, why would RCU care and allocate more memory?

#4 Similar to #3. If the interrupt handler loops forever or if the
interrupt is a runaway which prevents task/softirq processing then RCU free performance is the least of your worries.

#5 Clearly a bug and making RCU accomodate for that is beyond silly.

So if call_rcu() detects that the remaining storage space for pointers goes below the critical point or if it observes high frequency calls then it simply should force a soft interrupt which does the allocation.

Allocating from softirq context obviously without holding the raw lock which is used inside call_rcu() is safe on all configurations.

If call_rcu() is forced to use the fallback for a few calls until this happens then that's not the end of the world. It is not going to be a problem ever for the most obvious issue #1, user space madness, because that case cannot delay the softirq processing unless there is a kernel bug which makes again RCU free performance irrelevant.

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

That would make it trivial as we could simply switch it over to rcu_wait.

Quoted 7 lines

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 6 lines

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

PeterZ might have opinions about that too I suspect.

Quoted 32 lines · Thomas Gleixner

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Thomas Gleixner

It basically requires to convert the wait queue to something else. Is the waitqueue strict single waiter?

I would have to double check. From what I remember only kswapd should ever sleep on it.

That would make it trivial as we could simply switch it over to rcu_wait.

Quoted 7 lines

Quoted 7 lines

So that should be:

	if (!preemptible() && gfp == GFP_RT_NOWAIT)

which is limiting the damage to those callers which hand in GFP_RT_NOWAIT.

Quoted 6 lines

Quoted 3 lines

lockdep will yell at invocations with gfp != GFP_RT_NOWAIT when it hits zone->lock in the wrong context. And we want to know about that so we can look at the caller and figure out how to solve it.

Yes, that would have to somehow need to annotate the zone_lock to be ok in those paths so that lockdep doesn't complain.

That opens the worst of all cans of worms. If we start this here then Joe programmer and his dog will use these lockdep annotation to evade warnings and when exposed to RT it will fall apart in pieces. Just that at that point Joe programmer moved on to something else and the usual suspects can mop up the pieces. We've seen that all over the place and some people even disable lockdep temporarily because annotations don't help.

Quoted 1 line

PeterZ might have opinions about that too I suspect.

PeterZ is mightily confused by all of this -- also heat induced brain melt.

I thought the rule was:

- No allocators (alloc/free) inside raw_spinlock_t, full-stop.

Why are we trying to craft an exception?

Quoted 5 lines · On Thu, Aug 13, 2020 at 08:26:18PM +0200, [email protected] wrote:

I thought the rule was:

- No allocators (alloc/free) inside raw_spinlock_t, full-stop.

Why are we trying to craft an exception?

So that we can reduce post-grace-period cache misses by a factor of eight when invoking RCU callbacks. This reduction in cache misses also makes it more difficult to overrun RCU with floods of either call_rcu() or kfree_rcu() invocations.

The idea is to allocate page-sized arrays of pointers so that the callback invocation can sequence through the array instead of walking a linked list, hence the reduction in cache misses.

Quoted 13 lines · Paul E. McKenney

Quoted 5 lines · On Thu, Aug 13, 2020 at 08:26:18PM +0200, [email protected] wrote:

I thought the rule was:

- No allocators (alloc/free) inside raw_spinlock_t, full-stop.

Why are we trying to craft an exception?

So that we can reduce post-grace-period cache misses by a factor of eight when invoking RCU callbacks. This reduction in cache misses also makes it more difficult to overrun RCU with floods of either call_rcu() or kfree_rcu() invocations.

Quoted 3 lines

The idea is to allocate page-sized arrays of pointers so that the callback invocation can sequence through the array instead of walking a linked list, hence the reduction in cache misses.

I'm still not getting it, how do we end up trying to allocate memory from under raw spinlocks if you're not allowed to use kfree_rcu() under one ?

Can someone please spell out the actual problem?

I'm still not getting it, how do we end up trying to allocate memory from under raw spinlocks if you're not allowed to use kfree_rcu() under one ?

Can someone please spell out the actual problem?

1. Prohibit invoking allocators from raw atomic context, such
as when holding a raw spinlock.

2. Adding a GFP_ flag.

3. Reusing existing GFP_ flags/values/whatever to communicate
the raw-context information that was to be communicated by the new GFP_ flag.

4. Making lockdep forgive acquiring spinlocks while holding
raw spinlocks, but only in CONFIG_PREEMPT_NONE=y kernels.

Quoted 5 lines · peterz

I'm still not getting it, how do we end up trying to allocate memory from under raw spinlocks if you're not allowed to use kfree_rcu() under one ?

Can someone please spell out the actual problem?

Your actual problem is the heat wave. Get an icepack already :)

To set things straight:

1) kfree_rcu() which used to be just a conveniance wrapper around call_rcu() always worked in any context except NMI.

Otherwise RT would have never worked or would have needed other horrible hacks to defer kfree in certain contexts including context switch.

2) RCU grew an optimization for kfree_rcu() which avoids the linked list cache misses when a large number of objects is freed via kfree_rcu(). Paul says it speeds up post grace period processing by a factor of 8 which is impressive.

That's done by avoiding the linked list and storing the object pointer in an array of pointers which is page sized. This array is then freed in bulk without having to touch the rcu head linked list which obviously avoids cache misses.

Occasionally kfree_rcu() needs to allocate a page for this but it can fallback to the linked list when the allocation fails.

Inconveniantly this allocation happens with an RCU internal raw lock held, but even if we could do the allocation outside the RCU internal lock this would not solve the problem that kfree_rcu() can be called in any context except NMI even on RT.

So RT forbids allocations from truly atomic contexts even with GFP_ATOMIC/NOWAIT. GFP_ATOMIC is kinda meaningless on RT because everything which calls it is in non-atomic context :) But still GFP_ATOMIC or GFP_NOWAIT retain the semantics of !RT and do not go down into deeper levels or wait for pages to become available.

3) #2 upsets lockdep (with the raw lock nesting enabled) rightfully when RCU tries to allocate a page, the lockless page cache is empty and it acquires zone->lock which is a regular spinlock

4) As kfree_rcu() can be used from any context except NMI and RT relies on it we ran into a circular dependency problem.

If we disable that feature for RT then the problem would be solved except that lockdep still would complain about taking zone->lock within a forbidden context on !RT kernels.

Preventing that feature because of that is not a feasible option either. Aside of that we discuss this postfactum, IOW the stuff is upstream already despite the problem being known before.

5) Ulad posted patches which introduce a new GFP allocation mode which makes the allocation fail if the lockless cache is empty, i.e. it does not try to touch zone->lock in that case.

That works perfectly fine on RT and !RT, makes lockdep happy and Paul is happy as well.

If the lockless cache, which works perfectly fine on RT, is empty then the performance of kfree_rcu() post grace period processing is probably not making the situation of the system worse.

Michal is not fond of the new GFP flag and wants to explore options to avoid that completely. But so far there is no real feasible solution.

A) It was suggested to make zone->lock raw, but that's a total disaster latency wise. With just a kernel compile (!RT kernel) spinwait times are in the hundreds of microseconds.

B) Michal suggested to have GFP_RT_ATOMIC defined to 0 which would not require a new GFP bit and bail out when RT is enabled and the context is atomic.

D) To solve the lockdep issue of #B Michal suggested to have magic lockdep annotations which allow !RT kernels to take zone->lock from the otherwise forbidden contexts because on !RT this lock nesting could be considered a false positive.

But this would be horrors of some sorts because there might be locks further down which then need the same treatment or some general 'yay, I'm excempt from everything' annotation which is short of disabling lockdep completly.

Even if that could be solved and made correct for both RT and !RT then this opens the can of worms that this kind of annotation would show up all over the place within no time for the completely wrong reasons.

Paul compiled this options list:

Quoted 2 lines

1. Prohibit invoking allocators from raw atomic context, such
as when holding a raw spinlock.

Clearly the simplest solution but not Pauls favourite and unfortunately he has a good reason.

Quoted 1 line

2. Adding a GFP_ flag.

Michal does not like the restriction for !RT kernels and tries to avoid the introduction of a new allocation mode.

Quoted 6 lines

3. Reusing existing GFP_ flags/values/whatever to communicate
the raw-context information that was to be communicated by the new GFP_ flag.

4. Making lockdep forgive acquiring spinlocks while holding
raw spinlocks, but only in CONFIG_PREEMPT_NONE=y kernels.

These are not seperate of each other as #3 requires #4. The most horrible solution IMO from a technical POV as it proliferates inconsistency for no good reaosn.

Aside of that it'd be solving a problem which does not exist simply because kfree_rcu() does not depend on it and we really don't want to set precedence and encourage the (ab)use of this in any way.

Quoted 5 lines

Quoted 2 lines

1. Prohibit invoking allocators from raw atomic context, such
as when holding a raw spinlock.

Clearly the simplest solution but not Pauls favourite and unfortunately he has a good reason.

Which isn't actually stated anywhere I suppose ?

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines

Quoted 2 lines

1. Prohibit invoking allocators from raw atomic context, such
as when holding a raw spinlock.

Clearly the simplest solution but not Pauls favourite and unfortunately he has a good reason.

Which isn't actually stated anywhere I suppose ?

Introduce raw_kfree_rcu() that doesn't do the allocation, and fix the few wonky callsites.

Quoted 8 lines · Thomas Gleixner

Quoted 5 lines · peterz

I'm still not getting it, how do we end up trying to allocate memory from under raw spinlocks if you're not allowed to use kfree_rcu() under one ?

Can someone please spell out the actual problem?

Your actual problem is the heat wave. Get an icepack already :)

Sure, but also much of the below wasn't stated anywhere in the thread I got Cc'ed on. All I got was half arsed solutions without an actual problem statement.

Quoted 2 lines

To set things straight:

Quoted 6 lines

1) kfree_rcu() which used to be just a conveniance wrapper around call_rcu() always worked in any context except NMI.

Otherwise RT would have never worked or would have needed other horrible hacks to defer kfree in certain contexts including context switch.

I've never used kfree_rcu(), htf would I know.

Quoted 18 lines

2) RCU grew an optimization for kfree_rcu() which avoids the linked list cache misses when a large number of objects is freed via kfree_rcu(). Paul says it speeds up post grace period processing by a factor of 8 which is impressive.

That's done by avoiding the linked list and storing the object pointer in an array of pointers which is page sized. This array is then freed in bulk without having to touch the rcu head linked list which obviously avoids cache misses.

Occasionally kfree_rcu() needs to allocate a page for this but it can fallback to the linked list when the allocation fails.

Inconveniantly this allocation happens with an RCU internal raw lock held, but even if we could do the allocation outside the RCU internal lock this would not solve the problem that kfree_rcu() can be called in any context except NMI even on RT.

Quoted 5 lines

So RT forbids allocations from truly atomic contexts even with GFP_ATOMIC/NOWAIT. GFP_ATOMIC is kinda meaningless on RT because everything which calls it is in non-atomic context :) But still GFP_ATOMIC or GFP_NOWAIT retain the semantics of !RT and do not go down into deeper levels or wait for pages to become available.

Right so on RT just cut out the allocation and unconditionally make it NULL. Since it needs to deal with GFP_ATOMIC|GFP_NOWARN failing the allocation anyway.

Quoted 3 lines

3) #2 upsets lockdep (with the raw lock nesting enabled) rightfully when RCU tries to allocate a page, the lockless page cache is empty and it acquires zone->lock which is a regular spinlock

There was no lockdep splat in the thread.

Quoted 2 lines

4) As kfree_rcu() can be used from any context except NMI and RT relies on it we ran into a circular dependency problem.

Well, which actual usage sites are under a raw spinlock? Most of the ones I could find are not.

Quoted 7 lines

If we disable that feature for RT then the problem would be solved except that lockdep still would complain about taking zone->lock within a forbidden context on !RT kernels.

Preventing that feature because of that is not a feasible option either. Aside of that we discuss this postfactum, IOW the stuff is upstream already despite the problem being known before.

Well, that's a fail :-( I tought we were supposed to solve known issues before shit got merged.

Quoted 11 lines

5) Ulad posted patches which introduce a new GFP allocation mode which makes the allocation fail if the lockless cache is empty, i.e. it does not try to touch zone->lock in that case.

That works perfectly fine on RT and !RT, makes lockdep happy and Paul is happy as well.

If the lockless cache, which works perfectly fine on RT, is empty then the performance of kfree_rcu() post grace period processing is probably not making the situation of the system worse.

Quoted 3 lines

Michal is not fond of the new GFP flag and wants to explore options to avoid that completely. But so far there is no real feasible solution.

Not only Michal, those patches looked pretty horrid.

Quoted 3 lines

A) It was suggested to make zone->lock raw, but that's a total disaster latency wise. With just a kernel compile (!RT kernel) spinwait times are in the hundreds of microseconds.

Yeah, I know, been there done that, like over a decade ago :-)

Quoted 3 lines

B) Michal suggested to have GFP_RT_ATOMIC defined to 0 which would not require a new GFP bit and bail out when RT is enabled and the context is atomic.

I would've written the code like:

	void *mem = NULL;

	...
#ifndef CONFIG_PREEMPT_RT
	mem = kmalloc(PAGE_SIZE, GFP_ATOMIC|GFP_NOWAIT);
#endif
	if (!mem)
	  ....

Seems much simpler and doesn't pollute the GFP_ space.

Quoted 10 lines

D) To solve the lockdep issue of #B Michal suggested to have magic lockdep annotations which allow !RT kernels to take zone->lock from the otherwise forbidden contexts because on !RT this lock nesting could be considered a false positive.

But this would be horrors of some sorts because there might be locks further down which then need the same treatment or some general 'yay, I'm excempt from everything' annotation which is short of disabling lockdep completly.

Quoted 4 lines

Even if that could be solved and made correct for both RT and !RT then this opens the can of worms that this kind of annotation would show up all over the place within no time for the completely wrong reasons.

So due to this heat crap I've not slept more than a few hours a night for the last week, I'm not entirely there, but we already have a bunch of lockdep magic for this raw nesting stuff.

But yes, we need to avoid abuse, grep for lockdep_off() :-( This drivers/md/dm-cache-target.c thing is just plain broken. It sorta 'works' on mainline (and even there it can behave badly), but on RT it will come apart with a bang.

Quoted 2 lines

Paul compiled this options list:

Quoted 5 lines

Quoted 2 lines

1. Prohibit invoking allocators from raw atomic context, such
as when holding a raw spinlock.

Clearly the simplest solution but not Pauls favourite and unfortunately he has a good reason.

Which isn't actually stated anywhere I suppose ?

Quoted 4 lines

Quoted 1 line

2. Adding a GFP_ flag.

Michal does not like the restriction for !RT kernels and tries to avoid the introduction of a new allocation mode.

Like above, I tend to be with Michal on this, just wrap the actual allocation in CONFIG_PREEMPT_RT, the code needs to handle a NULL pointer there anyway.

Quoted 6 lines

Quoted 6 lines

3. Reusing existing GFP_ flags/values/whatever to communicate
the raw-context information that was to be communicated by the new GFP_ flag.

4. Making lockdep forgive acquiring spinlocks while holding
raw spinlocks, but only in CONFIG_PREEMPT_NONE=y kernels.

Uhh, !CONFIG_PREEMPT_RT, the rest is 'fine'.

Quoted 7 lines

These are not seperate of each other as #3 requires #4. The most horrible solution IMO from a technical POV as it proliferates inconsistency for no good reaosn.

Aside of that it'd be solving a problem which does not exist simply because kfree_rcu() does not depend on it and we really don't want to set precedence and encourage the (ab)use of this in any way.

My preferred solution is 1, if you want to use an allocator, you simply don't get to play under raw_spinlock_t. And from a quick grep, most kfree_rcu() users are not under raw_spinlock_t context.

This here sounds like someone who wants to have his cake and eat it too.

I'll try and think about a lockdep annotation that isn't utter crap, but that's probably next week, I need this heat to end and get a few nights sleep, I'm an utter wreck atm.

Quoted 1 line · Peter Zijlstra

On Fri, Aug 14, 2020 at 01:59:04AM +0200, Thomas Gleixner wrote:

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

3. Reusing existing GFP_ flags/values/whatever to communicate
the raw-context information that was to be communicated by the new GFP_ flag.

4. Making lockdep forgive acquiring spinlocks while holding
raw spinlocks, but only in CONFIG_PREEMPT_NONE=y kernels.

Uhh, !CONFIG_PREEMPT_RT, the rest is 'fine'.

I would be OK with either. In CONFIG_PREEMPT_NONE=n kernels, the kfree_rcu() code could use preemptible() to determine whether it was safe to invoke the allocator. The code in kfree_rcu() might look like this:

	mem = NULL;
	if (IS_ENABLED(CONFIG_PREEMPT_NONE) || preemptible())
		mem = __get_free_page(...);

Is your point is that the usual mistakes would then be caught by the usual testing on CONFIG_PREEMPT_NONE=n kernels?

Just to make sure we are talking about the same thing, please see below for an untested patch that illustrates how I was interpreting your words. Was this what you had in mind?

Quoted 3 lines · Paul E. McKenney

Just to make sure we are talking about the same thing, please see below for an untested patch that illustrates how I was interpreting your words. Was this what you had in mind?

No, definitely not.

Also, since we used to be able to use call_rcu() _everywhere_, including under zone->lock, how's that working with you calling the page-allocating from it?

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines · Paul E. McKenney

Just to make sure we are talking about the same thing, please see below for an untested patch that illustrates how I was interpreting your words. Was this what you had in mind?

Quoted 5 lines

No, definitely not.

Also, since we used to be able to use call_rcu() _everywhere_, including under zone->lock, how's that working with you calling the page-allocating from it?

Indeed, that is exactly the problem we are trying to solve.

Quoted 8 lines · Paul E. McKenney

Quoted 7 lines · Peter Zijlstra

Quoted 3 lines · Paul E. McKenney

Just to make sure we are talking about the same thing, please see below for an untested patch that illustrates how I was interpreting your words. Was this what you had in mind?

No, definitely not.

Quoted 5 lines

Quoted 3 lines

Also, since we used to be able to use call_rcu() _everywhere_, including under zone->lock, how's that working with you calling the page-allocating from it?

Indeed, that is exactly the problem we are trying to solve.

Wait a moment. Why are we discussing RT induced raw non raw lock ordering at all?

Whatever kernel you variant you look at this is not working:

lock(zone) call_rcu() lock(zone)

It's a simple recursive dead lock, nothing else.

And that enforces the GFP_NOLOCK allocation mode or some other solution unless you make a new rule that calling call_rcu() is forbidden while holding zone lock or any other lock which might be nested inside the GFP_NOWAIT zone::lock held region.

Quoted 4 lines

And that enforces the GFP_NOLOCK allocation mode or some other solution unless you make a new rule that calling call_rcu() is forbidden while holding zone lock or any other lock which might be nested inside the GFP_NOWAIT zone::lock held region.

Again, you are correct. Maybe the forecasted weekend heat will cause my brain to hallucinate a better solution, but in the meantime, the GFP_NOLOCK approach looks good from this end.

Quoted 8 lines · Paul E. McKenney

Quoted 4 lines

And that enforces the GFP_NOLOCK allocation mode or some other solution unless you make a new rule that calling call_rcu() is forbidden while holding zone lock or any other lock which might be nested inside the GFP_NOWAIT zone::lock held region.

Again, you are correct. Maybe the forecasted weekend heat will cause my brain to hallucinate a better solution, but in the meantime, the GFP_NOLOCK approach looks good from this end.

So I hate __GFP_NO_LOCKS for a whole number of reasons:

- it should be called __GFP_LOCKLESS if anything
- it sprinkles a bunch of ugly branches around the allocator fast path
- it only works for order==0

Combined I really odn't think this should be a GFP flag. How about a special purpose allocation function, something like so..

---

mm/page_alloc.c +41 -0

index 901a21f61d68..cdec9c99fba7 100644
@@ -4875,6 +4875,47 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order, int preferred_nid,
 }
 EXPORT_SYMBOL(__alloc_pages_nodemask);
 
+struct page *__rmqueue_lockless(struct zone *zone, struct per_cpu_pages *pcp)
+{
+	struct list_head *list;
+	struct page *page;
+	int migratetype;
+
+	for (migratetype = 0; migratetype < MIGRATE_PCPTYPES; migratetype++) {
+		list = &pcp->list[migratetype];
+		page = list_first_entry_or_null(list, struct page, lru);
+		if (page && check_new_pcp(page)) {
+			list_del(&page->lru);
+			pcp->count--;
+			return page;
+		}
+	}
+
+	return NULL;
+}
+
+struct page *__alloc_page_lockless(void)
+{
+	struct zonelist *zonelist = node_zonelist(numa_node_id(), GFP_KERNEL);
+	struct per_cpu_pages *pcp;
+	struct page *page = NULL;
+	unsigned long flags;
+	struct zoneref *z;
+	struct zone *zone;
+
+	for_each_zone_zonelist(zone, z, zonelist, ZONE_NORMAL) {
+		local_irq_save(flags);
+		pcp = &this_cpu_ptr(zone->pageset)->pcp;
+		page = __rmqueue_lockless(zone, pcp);
+		local_irq_restore(flags);
+
+		if (page)
+			break;
+	}
+
+	return page;
+}
+
 /*
  * Common helper functions. Never use with __GFP_HIGHMEM because the returned
  * address cannot represent highmem pages. Use alloc_pages and then kmap if

Quoted 8 lines · Paul E. McKenney

Quoted 4 lines

And that enforces the GFP_NOLOCK allocation mode or some other solution unless you make a new rule that calling call_rcu() is forbidden while holding zone lock or any other lock which might be nested inside the GFP_NOWAIT zone::lock held region.

Again, you are correct. Maybe the forecasted weekend heat will cause my brain to hallucinate a better solution, but in the meantime, the GFP_NOLOCK approach looks good from this end.

So I hate __GFP_NO_LOCKS for a whole number of reasons:

- it should be called __GFP_LOCKLESS if anything
- it sprinkles a bunch of ugly branches around the allocator fast path
- it only works for order==0

Combined I really odn't think this should be a GFP flag. How about a special purpose allocation function, something like so..

Quoted 8 lines · Paul E. McKenney

Quoted 4 lines

And that enforces the GFP_NOLOCK allocation mode or some other solution unless you make a new rule that calling call_rcu() is forbidden while holding zone lock or any other lock which might be nested inside the GFP_NOWAIT zone::lock held region.

Again, you are correct. Maybe the forecasted weekend heat will cause my brain to hallucinate a better solution, but in the meantime, the GFP_NOLOCK approach looks good from this end.

So I hate __GFP_NO_LOCKS for a whole number of reasons:

- it should be called __GFP_LOCKLESS if anything
- it sprinkles a bunch of ugly branches around the allocator fast path
- it only works for order==0

Combined I really odn't think this should be a GFP flag. How about a special purpose allocation function, something like so..

Michal asked to provide some data regarding how many pages we need and how "lockless allocation" behaves when it comes to success vs failed scenarios.

Please see below some results. The test case is a tight loop of 1 000 000 allocations doing kmalloc() and kfree_rcu():

sudo ./test_vmalloc.sh run_test_mask=2048 single_cpu_test=1

<snip>
for (i = 0; i < 1 000 000; i++) {
p = kmalloc(sizeof(*p), GFP_KERNEL);
if (!p)
return -1;

p->array[0] = 'a';
kvfree_rcu(p, rcu);
}
<snip>

wget ftp://vps418301.ovh.net/incoming/1000000_kmalloc_kfree_rcu_proc_percpu_pagelist_fractio_is_0.png

wget ftp://vps418301.ovh.net/incoming/1000000_kmalloc_kfree_rcu_proc_percpu_pagelist_fractio_is_8.png

Also i would like to underline, that kfree_rcu() reclaim logic can be improved further, making the drain logic more efficient when it comes to time, thus to reduce a footprint as a result number of required pages.

--
Vlad Rezki

On Mon 17-08-20 00:56:55, Uladzislau Rezki wrote:
[...]

Quoted 5 lines

Michal asked to provide some data regarding how many pages we need and how "lockless allocation" behaves when it comes to success vs failed scenarios.

Please see below some results. The test case is a tight loop of 1 000 000 allocations doing kmalloc() and kfree_rcu():

It would be nice to cover some more realistic workloads as well.

Quoted 13 lines

sudo ./test_vmalloc.sh run_test_mask=2048 single_cpu_test=1

<snip>
for (i = 0; i < 1 000 000; i++) {
p = kmalloc(sizeof(*p), GFP_KERNEL);
if (!p)
return -1;

p->array[0] = 'a';
kvfree_rcu(p, rcu);
}
<snip>

Quoted 1 line

wget ftp://vps418301.ovh.net/incoming/1000000_kmalloc_kfree_rcu_proc_percpu_pagelist_fractio_is_0.png

If I understand this correctly then this means that failures happen very often because pcp pages are not recycled quicklly enough.

Quoted 1 line

wget ftp://vps418301.ovh.net/incoming/1000000_kmalloc_kfree_rcu_proc_percpu_pagelist_fractio_is_8.png

1/8 of the memory in pcp lists is quite large and likely not something used very often.

Both these numbers just make me think that a dedicated pool of page pre-allocated for RCU specifically might be a better solution. I still haven't read through that branch of the email thread though so there might be some pretty convincing argments to not do that.

Quoted 6 lines

Also i would like to underline, that kfree_rcu() reclaim logic can be improved further, making the drain logic more efficient when it comes to time, thus to reduce a footprint as a result number of required pages.

--
Vlad Rezki

Signature · 2 lines

Michal Hocko
SUSE Labs

On Mon 17-08-20 00:56:55, Uladzislau Rezki wrote:

Quoted 1 line

wget ftp://vps418301.ovh.net/incoming/1000000_kmalloc_kfree_rcu_proc_percpu_pagelist_fractio_is_8.png

1/8 of the memory in pcp lists is quite large and likely not something used very often.

Both these numbers just make me think that a dedicated pool of page pre-allocated for RCU specifically might be a better solution. I still haven't read through that branch of the email thread though so there might be some pretty convincing argments to not do that.

Quoted 1 line · Michal Hocko

On Mon 17-08-20 00:56:55, Uladzislau Rezki wrote:

[ . . . ]

Quoted 9 lines

Quoted 1 line

wget ftp://vps418301.ovh.net/incoming/1000000_kmalloc_kfree_rcu_proc_percpu_pagelist_fractio_is_8.png

1/8 of the memory in pcp lists is quite large and likely not something used very often.

Both these numbers just make me think that a dedicated pool of page pre-allocated for RCU specifically might be a better solution. I still haven't read through that branch of the email thread though so there might be some pretty convincing argments to not do that.

To avoid the problematic corner cases, we would need way more dedicated memory than is reasonable, as in well over one hundred pages per CPU. Sure, we could choose a smaller number, but then we are failing to defend against flooding, even on systems that have more than enough free memory to be able to do so. It would be better to live within what is available, taking the performance/robustness hit only if there isn't enough.

My current belief is that we need a combination of (1) either the GFP_NOLOCK flag or Peter Zijlstra's patch and

Quoted 15 lines · Paul E. McKenney

Quoted 1 line · Michal Hocko

On Mon 17-08-20 00:56:55, Uladzislau Rezki wrote:

[ . . . ]

Quoted 9 lines

Quoted 1 line

wget ftp://vps418301.ovh.net/incoming/1000000_kmalloc_kfree_rcu_proc_percpu_pagelist_fractio_is_8.png

1/8 of the memory in pcp lists is quite large and likely not something used very often.

Both these numbers just make me think that a dedicated pool of page pre-allocated for RCU specifically might be a better solution. I still haven't read through that branch of the email thread though so there might be some pretty convincing argments to not do that.

Quoted 6 lines

To avoid the problematic corner cases, we would need way more dedicated memory than is reasonable, as in well over one hundred pages per CPU. Sure, we could choose a smaller number, but then we are failing to defend against flooding, even on systems that have more than enough free memory to be able to do so. It would be better to live within what is available, taking the performance/robustness hit only if there isn't enough.

Thomas had a good point that it doesn't really make much sense to optimize for flooders because that just makes them more effective.

Quoted 2 lines

My current belief is that we need a combination of (1) either the GFP_NOLOCK flag or Peter Zijlstra's patch and

I must have missed the patch?

Quoted 2 lines · Michal Hocko

Thomas had a good point that it doesn't really make much sense to optimize for flooders because that just makes them more effective.

The point is not to make the flooders go faster, but rather for the system to be robust in the face of flooders. Robust as in harder for a flooder to OOM the system.

And reducing the number of post-grace-period cache misses makes it easier for the callback-invocation-time memory freeing to keep up with the flooder, thus avoiding (or at least delaying) the OOM.

Quoted 8 lines · Paul E. McKenney

Quoted 2 lines · Michal Hocko

Thomas had a good point that it doesn't really make much sense to optimize for flooders because that just makes them more effective.

The point is not to make the flooders go faster, but rather for the system to be robust in the face of flooders. Robust as in harder for a flooder to OOM the system.

Quoted 3 lines

And reducing the number of post-grace-period cache misses makes it easier for the callback-invocation-time memory freeing to keep up with the flooder, thus avoiding (or at least delaying) the OOM.

Throttling the flooder is incresing robustness far more than reducing cache misses.

Quoted 13 lines · Thomas Gleixner

Quoted 11 lines · Paul E. McKenney

Quoted 2 lines · Michal Hocko

Thomas had a good point that it doesn't really make much sense to optimize for flooders because that just makes them more effective.

The point is not to make the flooders go faster, but rather for the system to be robust in the face of flooders. Robust as in harder for a flooder to OOM the system.

And reducing the number of post-grace-period cache misses makes it easier for the callback-invocation-time memory freeing to keep up with the flooder, thus avoiding (or at least delaying) the OOM.

Quoted 2 lines

Throttling the flooder is incresing robustness far more than reducing cache misses.

True, but it takes time to identify a flooding event that needs to be throttled (as in milliseconds). This time cannot be made up.

Quoted 17 lines · Paul E. McKenney

Quoted 15 lines · Thomas Gleixner

Quoted 11 lines · Paul E. McKenney

Quoted 2 lines · Michal Hocko

Thomas had a good point that it doesn't really make much sense to optimize for flooders because that just makes them more effective.

The point is not to make the flooders go faster, but rather for the system to be robust in the face of flooders. Robust as in harder for a flooder to OOM the system.

And reducing the number of post-grace-period cache misses makes it easier for the callback-invocation-time memory freeing to keep up with the flooder, thus avoiding (or at least delaying) the OOM.

Throttling the flooder is incresing robustness far more than reducing cache misses.

Quoted 2 lines

True, but it takes time to identify a flooding event that needs to be throttled (as in milliseconds). This time cannot be made up.

Not really. A flooding event will deplete your preallocated pages very fast, so you have to go into the allocator and get new ones which naturally throttles the offender.

So if your open/close thing uses the new single argument free which has to be called from sleepable context then the allocation either gives you a page or that thing has to wait. No fancy extras.

You still can have a page reserved for your other regular things and once that it gone, you have to fall back to the linked list for those. But when that happens the extra cache misses are not your main problem anymore.

Quoted 8 lines · Thomas Gleixner

Quoted 6 lines · Paul E. McKenney

Quoted 2 lines · Thomas Gleixner

Throttling the flooder is incresing robustness far more than reducing cache misses.

True, but it takes time to identify a flooding event that needs to be throttled (as in milliseconds). This time cannot be made up.

Quoted 3 lines

Not really. A flooding event will deplete your preallocated pages very fast, so you have to go into the allocator and get new ones which naturally throttles the offender.

Should it turn out that we can in fact go into the allocator, completely agreed.

Quoted 3 lines

So if your open/close thing uses the new single argument free which has to be called from sleepable context then the allocation either gives you a page or that thing has to wait. No fancy extras.

In the single-argument kvfree_rcu() case, completely agreed.

Quoted 4 lines

You still can have a page reserved for your other regular things and once that it gone, you have to fall back to the linked list for those. But when that happens the extra cache misses are not your main problem anymore.

The extra cache misses are a problem in that case because they throttle the reclamation, which anti-throttles the producer, especially in the case where callback invocation is offloaded.

Paul,

Quoted 9 lines · Paul E. McKenney

Quoted 8 lines · Thomas Gleixner

Quoted 6 lines · Paul E. McKenney

Quoted 2 lines · Thomas Gleixner

Throttling the flooder is incresing robustness far more than reducing cache misses.

True, but it takes time to identify a flooding event that needs to be throttled (as in milliseconds). This time cannot be made up.

Quoted 6 lines

Quoted 3 lines

Not really. A flooding event will deplete your preallocated pages very fast, so you have to go into the allocator and get new ones which naturally throttles the offender.

Should it turn out that we can in fact go into the allocator, completely agreed.

You better can for any user space controllable flooding source.