kvfree_rcu() and _LOCK_NESTING/_PREEMPT_RT

Proposal
========
Introduce a lock-free function that obtain a page from the per-cpu-lists on current CPU. It returns NULL rather than acquiring any non-raw spinlock.

On Fri 18-09-20 21:48:15, Uladzislau Rezki (Sony) wrote:
[...]

Quoted 4 lines

Proposal
========
Introduce a lock-free function that obtain a page from the per-cpu-lists on current CPU. It returns NULL rather than acquiring any non-raw spinlock.

I was not happy about this solution when we have discussed this last time and I have to say I am still not happy. This is exposing an internal allocator optimization and allows a hard to estimate consumption of pcp free pages. IIUC this run on pcp cache can be controled directly from userspace (close(open) loop IIRC) which makes it even bigger no-no.

I strongly agree with Thomas http://lkml.kernel.org/r/[email protected]
that this optimization is not aiming at reasonable workloads. Really, go
with pre-allocated buffer and fallback to whatever slow path you have already. Exposing more internals of the allocator is not going to do any
good for long term maintainability.

Quoted 7 lines · Michal Hocko

On Fri 18-09-20 21:48:15, Uladzislau Rezki (Sony) wrote:
[...]

Quoted 4 lines

Proposal
========
Introduce a lock-free function that obtain a page from the per-cpu-lists on current CPU. It returns NULL rather than acquiring any non-raw spinlock.

Quoted 6 lines

I was not happy about this solution when we have discussed this last time and I have to say I am still not happy. This is exposing an internal allocator optimization and allows a hard to estimate consumption of pcp free pages. IIUC this run on pcp cache can be controled directly from userspace (close(open) loop IIRC) which makes it even bigger no-no.

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

Quoted 5 lines

I strongly agree with Thomas http://lkml.kernel.org/r/[email protected]
that this optimization is not aiming at reasonable workloads. Really, go
with pre-allocated buffer and fallback to whatever slow path you have already. Exposing more internals of the allocator is not going to do any
good for long term maintainability.

I suggest that you carefully re-read the thread following that email.

Given a choice between making users unhappy and making developers unhappy, I will side with the users each and every time.

Quoted 6 lines

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

No worries I do not feel like a whipping boy here. But do expect me to argue against the approach. I would also appreciate it if there was some more information on other attempts, why they have failed. E.g. why pre-allocation is not an option that works well enough in most reasonable workloads.

I would also appreciate some more thoughts why we
need to optimize for heavy abusers of RCU (like close(open) extremes).

Quoted 7 lines

Quoted 5 lines

I strongly agree with Thomas http://lkml.kernel.org/r/[email protected]
that this optimization is not aiming at reasonable workloads. Really, go
with pre-allocated buffer and fallback to whatever slow path you have already. Exposing more internals of the allocator is not going to do any
good for long term maintainability.

I suggest that you carefully re-read the thread following that email.

I clearly remember Thomas not being particularly happy that you optimize for a corner case. I do not remember there being a consensus that this is the right approach. There was some consensus that this is better than a gfp flag. Still quite bad though if you ask me.

Quoted 2 lines

Given a choice between making users unhappy and making developers unhappy, I will side with the users each and every time.

Well, let me rephrase. It is not only about me (as a developer) being unhappy but also all the side effects this would have for users when performance of their favorite workload declines for no apparent reason just because pcp caches are depleted by an unrelated process.

Hello, Michal.

Quoted 7 lines

Quoted 6 lines

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

Quoted 5 lines

No worries I do not feel like a whipping boy here. But do expect me to argue against the approach. I would also appreciate it if there was some more information on other attempts, why they have failed. E.g. why pre-allocation is not an option that works well enough in most reasonable workloads.

Pre-allocating has some drawbacks:

a) It is impossible to predict how many pages will be required to cover a demand that is controlled by different workloads on various systems.

b) Memory overhead since we do not know how much pages should be
preloaded: 100, 200 or 300

As for memory overhead, it is important to reduce it because of embedded devices like phones, where a low memory condition is a big issue. In that sense pre-allocating is something that we strongly would like to avoid.

Quoted 3 lines

I would also appreciate some more thoughts why we
need to optimize for heavy abusers of RCU (like close(open) extremes).

I think here is a small misunderstanding. Please note, that is not only about performance and corner cases. There is a single argument support of the kvfree_rcu(ptr), where maintaining an array in time is needed. The fallback of the single argument case is extrimely slow.

Single-argument details is here: https://lkml.org/lkml/2020/4/28/1626

Quoted 16 lines

Quoted 7 lines

Quoted 5 lines

I strongly agree with Thomas http://lkml.kernel.org/r/[email protected]
that this optimization is not aiming at reasonable workloads. Really, go
with pre-allocated buffer and fallback to whatever slow path you have already. Exposing more internals of the allocator is not going to do any
good for long term maintainability.

I suggest that you carefully re-read the thread following that email.

I clearly remember Thomas not being particularly happy that you optimize for a corner case. I do not remember there being a consensus that this is the right approach. There was some consensus that this is better than a gfp flag. Still quite bad though if you ask me.

Quoted 2 lines

Given a choice between making users unhappy and making developers unhappy, I will side with the users each and every time.

Quoted 5 lines

Well, let me rephrase. It is not only about me (as a developer) being unhappy but also all the side effects this would have for users when performance of their favorite workload declines for no apparent reason just because pcp caches are depleted by an unrelated process.

If depleted, we have a special worker that charge it. From the other hand, the pcplist can be depleted by its nature, what _is_ not wrong. But just in case we secure it since you had a concern about it.

Could you please specify a real test case or workload you are talking about?

Quoted 13 lines

Quoted 11 lines

Quoted 5 lines

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

No worries I do not feel like a whipping boy here. But do expect me to argue against the approach. I would also appreciate it if there was some more information on other attempts, why they have failed. E.g. why pre-allocation is not an option that works well enough in most reasonable workloads.

Pre-allocating has some drawbacks:

Quoted 3 lines

a) It is impossible to predict how many pages will be required to cover a demand that is controlled by different workloads on various systems.

Yes, this is not trivial but not a rocket science either. Remember that you are relying on a very dumb watermark based pcp pool from the allocator.

Mimicing a similar implementation shouldn't be all that hard and you will get your own pool which doesn't affect other page allocator users as much as a bonus.

Quoted 2 lines

b) Memory overhead since we do not know how much pages should be
preloaded: 100, 200 or 300

Does anybody who really needs this optimization actually cares about 300 pages?

Quoted 4 lines

As for memory overhead, it is important to reduce it because of embedded devices like phones, where a low memory condition is a big issue. In that sense pre-allocating is something that we strongly would like to avoid.

How big "machines" are we talking about here? I would expect that really tiny machines would have hard times to really fill up thousands of pages with pointers to free...

Would a similar scaling as the page allocator feasible. Really I mostly do care about shared nature of the pcp allocator list that one user can easily monopolize with this API.

Quoted 7 lines

Quoted 3 lines

I would also appreciate some more thoughts why we
need to optimize for heavy abusers of RCU (like close(open) extremes).

I think here is a small misunderstanding. Please note, that is not only about performance and corner cases. There is a single argument support of the kvfree_rcu(ptr), where maintaining an array in time is needed. The fallback of the single argument case is extrimely slow.

This should be part of the changelog.

Quoted 2 lines

Single-argument details is here: https://lkml.org/lkml/2020/4/28/1626

Error 501

Quoted 16 lines

Quoted 16 lines

Quoted 7 lines

Quoted 5 lines

I strongly agree with Thomas http://lkml.kernel.org/r/[email protected]
that this optimization is not aiming at reasonable workloads. Really, go
with pre-allocated buffer and fallback to whatever slow path you have already. Exposing more internals of the allocator is not going to do any
good for long term maintainability.

I suggest that you carefully re-read the thread following that email.

I clearly remember Thomas not being particularly happy that you optimize for a corner case. I do not remember there being a consensus that this is the right approach. There was some consensus that this is better than a gfp flag. Still quite bad though if you ask me.

Quoted 2 lines

Given a choice between making users unhappy and making developers unhappy, I will side with the users each and every time.

Quoted 8 lines

Quoted 5 lines

Well, let me rephrase. It is not only about me (as a developer) being unhappy but also all the side effects this would have for users when performance of their favorite workload declines for no apparent reason just because pcp caches are depleted by an unrelated process.

If depleted, we have a special worker that charge it. From the other hand, the pcplist can be depleted by its nature, what _is_ not wrong. But just in case we secure it since you had a concern about it.

pcp free lists should ever get empty when we run out of memory and need to reclaim. Otherwise they are constantly refilled/rebalanced on demand. The fact that you are refilling them from outside just suggest that you are operating on a wrong layer. Really, create your own pool of pages and rebalance them based on the workload.

Quoted 1 line

Could you please specify a real test case or workload you are talking about?

I am not a performance expert but essentially any memory allocator heavy workload might notice. I am pretty sure Mel would tell you more.

Quoted 4 lines

Mimicing a similar implementation shouldn't be all that hard and you will get your own pool which doesn't affect other page allocator users as much as a bonus.

I see your point Michal. As i mentioned before, it is important to avoid of having such own pools, because the aim is not to waste memory resources. A page will be returned back to "page allocator" as soon as a scheduler place our reclaim thread on a CPU and grace period is passed. So, the resource can be used for other needs. What is important.

Otherwise a memory footprint is increased what is bad for low memory conditions when OOM is involved. Just in case, it is a big issue for mobile devices.

Quoted 4 lines

Would a similar scaling as the page allocator feasible. Really I mostly do care about shared nature of the pcp allocator list that one user can easily monopolize with this API.

I see your concern. pcplist can be monopolized by already existing API:

    while (i < 100)
        __get_free_page(GFP_NOWAIT | __GFP_NOWARN);

Quoted 4 lines

Quoted 1 line

Single-argument details is here: https://lkml.org/lkml/2020/4/28/1626

Error 501

Could you please elaborate? Do not want to speculate :)

Quoted 17 lines

Quoted 16 lines

Quoted 11 lines

Quoted 5 lines

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

No worries I do not feel like a whipping boy here. But do expect me to argue against the approach. I would also appreciate it if there was some more information on other attempts, why they have failed. E.g. why pre-allocation is not an option that works well enough in most reasonable workloads.

Pre-allocating has some drawbacks:

a) It is impossible to predict how many pages will be required to cover a demand that is controlled by different workloads on various systems.

Quoted 4 lines

Yes, this is not trivial but not a rocket science either. Remember that you are relying on a very dumb watermark based pcp pool from the allocator.

We rely on it, indeed. If the pcp-cache is depleted our special work is triggered to charge our local cache(few pages) such way will also initiate the process of pre-featching pages from the buddy allocator populating the depleted pcp-cache. I do not have any concern here.

Quoted 5 lines

Mimicing a similar implementation shouldn't be all that hard and you will get your own pool which doesn't affect other page allocator users as much as a bonus.

I see your point Michal. As i mentioned before, it is important to avoid of having such own pools, because the aim is not to waste memory resources. A page will be returned back to "page allocator" as soon as a scheduler place our reclaim thread on a CPU and grace period is passed. So, the resource can be used for other needs. What is important.

Otherwise a memory footprint is increased what is bad for low memory conditions when OOM is involved.

Quoted 5 lines

Quoted 4 lines

As for memory overhead, it is important to reduce it because of embedded devices like phones, where a low memory condition is a big issue. In that sense pre-allocating is something that we strongly would like to avoid.

Quoted 4 lines

How big "machines" are we talking about here? I would expect that really tiny machines would have hard times to really fill up thousands of pages with pointers to free...

I mentioned above. We can not rely on static model. We would like to have a mechanism that gives back ASAP used pages to page allocator for other needs.

Quoted 5 lines

Would a similar scaling as the page allocator feasible. Really I mostly do care about shared nature of the pcp allocator list that one user can easily monopolize with this API.

I see your concern. pcplist can be monopolized by already existing API:

    while (i < 100)
        __get_free_page(GFP_NOWAIT | __GFP_NOWARN);

Quoted 17 lines · Uladzislau Rezki

Quoted 17 lines

Quoted 16 lines

Quoted 11 lines

Quoted 5 lines

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

No worries I do not feel like a whipping boy here. But do expect me to argue against the approach. I would also appreciate it if there was some more information on other attempts, why they have failed. E.g. why pre-allocation is not an option that works well enough in most reasonable workloads.

Pre-allocating has some drawbacks:

a) It is impossible to predict how many pages will be required to cover a demand that is controlled by different workloads on various systems.

Quoted 8 lines

Quoted 4 lines

Yes, this is not trivial but not a rocket science either. Remember that you are relying on a very dumb watermark based pcp pool from the allocator.

We rely on it, indeed. If the pcp-cache is depleted our special work is triggered to charge our local cache(few pages) such way will also initiate the process of pre-featching pages from the buddy allocator populating the depleted pcp-cache. I do not have any concern here.

It can interfere with ATOMIC allocations in critical paths in extreme circumstances as it potentially puts increased pressure on the emergency reserve as watermarks are bypassed. That adds to the risk of a functional failuure if reclaim fails to make progress. The number of pages are likely to be limited and unpredictable. As it uses any PCP type, it potentially causes fragmention issues. For the last point, the allocations may be transient in the RCU case now but not guaranteed forever. As the API is in gfp.h, it's open to abuse so the next guy that comes along and thinks "I am critical no matter what the name says" will cause problems. While you could argue that would be caught in review, plenty of GFP flag abuses made it through review.

Fundamentally, this is simply shifting the problem from RCU to the page allocator because of the locking arrangements and hazard of acquiring zone lock is a raw spinlock is held on RT. It does not even make the timing predictable as an empty PCU list (for example, a full drain in low memory situations) may mean the emergency path is hit anyway. About all it changes is the timing of when the emergency path is hit in some circumstances -- it's not fixing the problem, it's simply changing the shape.

Quoted 9 lines

Quoted 4 lines

Mimicing a similar implementation shouldn't be all that hard and you will get your own pool which doesn't affect other page allocator users as much as a bonus.

I see your point Michal. As i mentioned before, it is important to avoid of having such own pools, because the aim is not to waste memory resources. A page will be returned back to "page allocator" as soon as a scheduler place our reclaim thread on a CPU and grace period is passed. So, the resource can be used for other needs. What is important.

As the emergency path and synchronising can be hit no matter what, why not increase the pool temporarily after the emergency path is hit and shrink it again later if necessary?

Quoted 2 lines

Otherwise a memory footprint is increased what is bad for low memory conditions when OOM is involved.

OOM would only be a major factor if the size of the pools meant the machine could not even operate or at least was severely degraded. However, depleting the PCPU lists for RCU may slow kswapd making reclaim progress and cause an OOM in itself, or at least an intervention by a userspace monitor that kills non-critical applications in the background when memory pressure exists.

Quoted 9 lines

Quoted 9 lines

Quoted 4 lines

As for memory overhead, it is important to reduce it because of embedded devices like phones, where a low memory condition is a big issue. In that sense pre-allocating is something that we strongly would like to avoid.

How big "machines" are we talking about here? I would expect that really tiny machines would have hard times to really fill up thousands of pages with pointers to free...

Quoted 3 lines

I mentioned above. We can not rely on static model. We would like to have a mechanism that gives back ASAP used pages to page allocator for other needs.

After an emergency, temporarily increase the size of the pool to avoid hitting the emergency path again in the near future.

Quoted 4 lines

Quoted 4 lines

Would a similar scaling as the page allocator feasible. Really I mostly do care about shared nature of the pcp allocator list that one user can easily monopolize with this API.

Quoted 4 lines

I see your concern. pcplist can be monopolized by already existing API:

    while (i < 100)
        __get_free_page(GFP_NOWAIT | __GFP_NOWARN);

That's not the same class of abuse as it can go to the buddy lists to refill the correct PCP lists, avoid fragmentation issues, obeys watermarks and wakes kswapd if it's not awake already.

Quoted 7 lines

Fundamentally, this is simply shifting the problem from RCU to the page allocator because of the locking arrangements and hazard of acquiring zone lock is a raw spinlock is held on RT. It does not even make the timing predictable as an empty PCU list (for example, a full drain in low memory situations) may mean the emergency path is hit anyway. About all it changes is the timing of when the emergency path is hit in some circumstances -- it's not fixing the problem, it's simply changing the shape.

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Is there some way that we can locklessly allocate memory, but return failure instead of running down the emergency pool? A change to the loop that iterates over the migration types? Or to the loop that iterates over the zones? Something else?

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

Mimicing a similar implementation shouldn't be all that hard and you will get your own pool which doesn't affect other page allocator users as much as a bonus.

I see your point Michal. As i mentioned before, it is important to avoid of having such own pools, because the aim is not to waste memory resources. A page will be returned back to "page allocator" as soon as a scheduler place our reclaim thread on a CPU and grace period is passed. So, the resource can be used for other needs. What is important.

Quoted 3 lines

As the emergency path and synchronising can be hit no matter what, why not increase the pool temporarily after the emergency path is hit and shrink it again later if necessary?

If I understand what you are suggesting, this is in fact what Uladzislau's prototyped commit 8c0a1269709d ("rcu/tree: Add a work to allocate pages from regular context") on the -rcu "dev" branch is intended to do. The issue, as Uladislau noted above, is that scheduler delays can prevent these pool-increase actions until the point at which there is no memory.

Quoted 9 lines

Quoted 2 lines

Otherwise a memory footprint is increased what is bad for low memory conditions when OOM is involved.

OOM would only be a major factor if the size of the pools meant the machine could not even operate or at least was severely degraded. However, depleting the PCPU lists for RCU may slow kswapd making reclaim progress and cause an OOM in itself, or at least an intervention by a userspace monitor that kills non-critical applications in the background when memory pressure exists.

When under emergency conditions, we have one page allocated per 500 objects passed to kvfree_rcu(). So the increase in total allocated memory load due to this emergency path is quite small.

Quoted 13 lines

Quoted 12 lines

Quoted 9 lines

Quoted 4 lines

As for memory overhead, it is important to reduce it because of embedded devices like phones, where a low memory condition is a big issue. In that sense pre-allocating is something that we strongly would like to avoid.

How big "machines" are we talking about here? I would expect that really tiny machines would have hard times to really fill up thousands of pages with pointers to free...

I mentioned above. We can not rely on static model. We would like to have a mechanism that gives back ASAP used pages to page allocator for other needs.

Quoted 2 lines

After an emergency, temporarily increase the size of the pool to avoid hitting the emergency path again in the near future.

By which time we might well already be in OOM territory. The emergency situations can ramp up very quickly.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Would a similar scaling as the page allocator feasible. Really I mostly do care about shared nature of the pcp allocator list that one user can easily monopolize with this API.

Quoted 8 lines

Quoted 4 lines

I see your concern. pcplist can be monopolized by already existing API:

    while (i < 100)
        __get_free_page(GFP_NOWAIT | __GFP_NOWARN);

That's not the same class of abuse as it can go to the buddy lists to refill the correct PCP lists, avoid fragmentation issues, obeys watermarks and wakes kswapd if it's not awake already.

Good point, and we did try doing it this way. Unfortunately, in current !PREEMPT kernels, this approach can deadlock on one of the allocator locks via call_rcu(). This experience caused us to look at lockless allocator access. It also inspired the unconditional PREEMPT_COUNT approach, but that has its own detractors. (Yes, we are of course still persuing it as well.)

Quoted 8 lines

Quoted 7 lines

Fundamentally, this is simply shifting the problem from RCU to the page allocator because of the locking arrangements and hazard of acquiring zone lock is a raw spinlock is held on RT. It does not even make the timing predictable as an empty PCU list (for example, a full drain in low memory situations) may mean the emergency path is hit anyway. About all it changes is the timing of when the emergency path is hit in some circumstances -- it's not fixing the problem, it's simply changing the shape.

Quoted 6 lines

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

But it's very similar to what mempools are for.

Quoted 5 lines

Is there some way that we can locklessly allocate memory, but return failure instead of running down the emergency pool? A change to the loop that iterates over the migration types? Or to the loop that iterates over the zones? Something else?

Only by duplicating some of the logic in get_page_from_freelist would protect the reserves. Even if you had that, the contents of the pcp pools can easily be zero for the local CPU and you still get stuck.

Quoted 14 lines

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

Mimicing a similar implementation shouldn't be all that hard and you will get your own pool which doesn't affect other page allocator users as much as a bonus.

I see your point Michal. As i mentioned before, it is important to avoid of having such own pools, because the aim is not to waste memory resources. A page will be returned back to "page allocator" as soon as a scheduler place our reclaim thread on a CPU and grace period is passed. So, the resource can be used for other needs. What is important.

Quoted 3 lines

As the emergency path and synchronising can be hit no matter what, why not increase the pool temporarily after the emergency path is hit and shrink it again later if necessary?

Quoted 6 lines

If I understand what you are suggesting, this is in fact what Uladzislau's prototyped commit 8c0a1269709d ("rcu/tree: Add a work to allocate pages from regular context") on the -rcu "dev" branch is intended to do. The issue, as Uladislau noted above, is that scheduler delays can prevent these pool-increase actions until the point at which there is no memory.

Scheduler latency would be a problem. You would have to keep an emergency "rescue" pool that is enough to make slow progress even if no other memory is available until the worker takes action. The worker that allocates pages from regular context would be to increase the pool size enough so the emergency reserve does not have to be used again in the near future.

Quoted 10 lines

Quoted 9 lines

Quoted 2 lines

Otherwise a memory footprint is increased what is bad for low memory conditions when OOM is involved.

OOM would only be a major factor if the size of the pools meant the machine could not even operate or at least was severely degraded. However, depleting the PCPU lists for RCU may slow kswapd making reclaim progress and cause an OOM in itself, or at least an intervention by a userspace monitor that kills non-critical applications in the background when memory pressure exists.

Quoted 4 lines

When under emergency conditions, we have one page allocated per 500 objects passed to kvfree_rcu(). So the increase in total allocated memory load due to this emergency path is quite small.

Fair enough but any solution that depends on the PCP having even one page available is going to need a fallback option of some sort.

Quoted 16 lines

Quoted 15 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

As for memory overhead, it is important to reduce it because of embedded devices like phones, where a low memory condition is a big issue. In that sense pre-allocating is something that we strongly would like to avoid.

Quoted 7 lines

Quoted 4 lines

How big "machines" are we talking about here? I would expect that really tiny machines would have hard times to really fill up thousands of pages with pointers to free...

I mentioned above. We can not rely on static model. We would like to have a mechanism that gives back ASAP used pages to page allocator for other needs.

After an emergency, temporarily increase the size of the pool to avoid hitting the emergency path again in the near future.

Quoted 3 lines

By which time we might well already be in OOM territory. The emergency situations can ramp up very quickly.

And if the pcp lists are empty, this problem still exists -- pretty much anything that depends on the pcp lists always having pages is going to have a failure case.

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Would a similar scaling as the page allocator feasible. Really I mostly do care about shared nature of the pcp allocator list that one user can easily monopolize with this API.

Quoted 8 lines

Quoted 4 lines

I see your concern. pcplist can be monopolized by already existing API:

    while (i < 100)
        __get_free_page(GFP_NOWAIT | __GFP_NOWARN);

That's not the same class of abuse as it can go to the buddy lists to refill the correct PCP lists, avoid fragmentation issues, obeys watermarks and wakes kswapd if it's not awake already.

Quoted 7 lines

Good point, and we did try doing it this way. Unfortunately, in current !PREEMPT kernels, this approach can deadlock on one of the allocator locks via call_rcu(). This experience caused us to look at lockless allocator access. It also inspired the unconditional PREEMPT_COUNT approach, but that has its own detractors. (Yes, we are of course still persuing it as well.)

I didn't keep up to date unfortunately but pretty much anything that needs guaranteed access to pages has to maintain a mempool of some sort. Even if the page allocator had a common pool for emergency use, it would be subject to abuse because all it would take is one driver or subsystem deciding that it was special enough to deplete it. For example, the ml4 driver decided that it should declare itself to be a "memory allocator" like kswapd for iSCSI (it claims swap-over-nfs, but swap-over-nfs was only meant to apply when a swapfile was backed by a network fs).

Other option is if we had unconditionally enabled PREEMPT_COUNT config. It would be easy to identify a context type and invoke a page allocator if a context is preemtale. But as of now preemptable() is "half" working. Thomas uploaded patches to make it unconditional. But it can be blocked.

Quoted 4 lines · Uladzislau Rezki

Other option is if we had unconditionally enabled PREEMPT_COUNT config. It would be easy to identify a context type and invoke a page allocator if a context is preemtale. But as of now preemptable() is "half" working. Thomas uploaded patches to make it unconditional. But it can be blocked.

While I in principle support Thomas' patch, I think this is an abuse and still complete wrong.

Not all preemptible() context can deal with GFP_KERNEL (GFP_NOFS existing should be a clue). !preemptible context cannot unconditionally deal with GFP_NOWAIT, consider using it while holding zone->lock.

The preemption context does not say antying much useful about the allocation context.

The key point is "enough". We need pages to make a) fast progress b) support single argument of kvfree_rcu(one_arg). Not vice versa. That "enough" depends on scheduler latency and vague pre-allocated number of pages, it might be not enough what would require to refill it more and more or we can overshoot that would lead to memory overhead. So we have here timing issues and not accurate model. IMHO.

Quoted 6 lines · Uladzislau Rezki

The key point is "enough". We need pages to make a) fast progress b) support single argument of kvfree_rcu(one_arg). Not vice versa. That "enough" depends on scheduler latency and vague pre-allocated number of pages, it might be not enough what would require to refill it more and more or we can overshoot that would lead to memory overhead. So we have here timing issues and not accurate model. IMHO.

I'm firmly opposed to the single argument kvfree_rcu() idea, that's requiring memory to free memory.

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Uladzislau Rezki

The key point is "enough". We need pages to make a) fast progress b) support single argument of kvfree_rcu(one_arg). Not vice versa. That "enough" depends on scheduler latency and vague pre-allocated number of pages, it might be not enough what would require to refill it more and more or we can overshoot that would lead to memory overhead. So we have here timing issues and not accurate model. IMHO.

Quoted 3 lines

I'm firmly opposed to the single argument kvfree_rcu() idea, that's requiring memory to free memory.

Hmm.. The problem is there is a demand in it:

Quoted 9 lines · Uladzislau Rezki

Quoted 8 lines · Peter Zijlstra

Quoted 6 lines · Uladzislau Rezki

The key point is "enough". We need pages to make a) fast progress b) support single argument of kvfree_rcu(one_arg). Not vice versa. That "enough" depends on scheduler latency and vague pre-allocated number of pages, it might be not enough what would require to refill it more and more or we can overshoot that would lead to memory overhead. So we have here timing issues and not accurate model. IMHO.

Quoted 4 lines

Quoted 3 lines

I'm firmly opposed to the single argument kvfree_rcu() idea, that's requiring memory to free memory.

Hmm.. The problem is there is a demand in it:

People demand ponies all the time, the usual answer is: No.

Quoted 6 lines · Uladzislau Rezki

The key point is "enough". We need pages to make a) fast progress b) support single argument of kvfree_rcu(one_arg). Not vice versa. That "enough" depends on scheduler latency and vague pre-allocated number of pages, it might be not enough what would require to refill it more and more or we can overshoot that would lead to memory overhead. So we have here timing issues and not accurate model. IMHO.

I'm firmly opposed to the single argument kvfree_rcu() idea, that's requiring memory to free memory.

Quoted 10 lines · Peter Zijlstra

Quoted 6 lines · Uladzislau Rezki

The key point is "enough". We need pages to make a) fast progress b) support single argument of kvfree_rcu(one_arg). Not vice versa. That "enough" depends on scheduler latency and vague pre-allocated number of pages, it might be not enough what would require to refill it more and more or we can overshoot that would lead to memory overhead. So we have here timing issues and not accurate model. IMHO.

I'm firmly opposed to the single argument kvfree_rcu() idea, that's requiring memory to free memory.

Not quite.

First, there is a fallback when memory allocation fails. Second, in heavy-use situations, there is only one allocation per about 500 kvfree_rcu() calls on 64-bit systems. Third, there are other long-standing situations that require allocating memory in order to free memory.

So I agree that it is a good general rule of thumb to avoid allocating on free paths, but there are exceptions. This is one of them.

Quoted 12 lines · Paul E. McKenney

Quoted 10 lines · Peter Zijlstra

Quoted 6 lines · Uladzislau Rezki

The key point is "enough". We need pages to make a) fast progress b) support single argument of kvfree_rcu(one_arg). Not vice versa. That "enough" depends on scheduler latency and vague pre-allocated number of pages, it might be not enough what would require to refill it more and more or we can overshoot that would lead to memory overhead. So we have here timing issues and not accurate model. IMHO.

I'm firmly opposed to the single argument kvfree_rcu() idea, that's requiring memory to free memory.

Quoted 7 lines

Not quite.

First, there is a fallback when memory allocation fails. Second, in heavy-use situations, there is only one allocation per about 500 kvfree_rcu() calls on 64-bit systems. Third, there are other long-standing situations that require allocating memory in order to free memory.

Some of which are quite broken. And yes, I'm aware of all that, I'm the one that started swap-over-NFS, which requires network traffic to free memory, which is one insane step further.

But the way to make that 'work' is carefully account and pre-allocate (or size the reserve) the required memory to make progress and to strictly limit concurrency to ensure you stay in your bounds.

Quoted 2 lines

So I agree that it is a good general rule of thumb to avoid allocating on free paths, but there are exceptions. This is one of them.

The very first thing you need to do is proof your memory usage is bounded, and then calculate your bound.

The problem is that with RCU you can't limit concurrency. call_rcu() can't block, you can't wait for a grace period to end when you've ran out of your reserve.

That is, you don't have a bound, so no reserve what so ever is going to help.

You must have that callback_head fallback.

Quoted 16 lines · Paul E. McKenney

Quoted 14 lines

Quoted 7 lines

Fundamentally, this is simply shifting the problem from RCU to the page allocator because of the locking arrangements and hazard of acquiring zone lock is a raw spinlock is held on RT. It does not even make the timing predictable as an empty PCU list (for example, a full drain in low memory situations) may mean the emergency path is hit anyway. About all it changes is the timing of when the emergency path is hit in some circumstances -- it's not fixing the problem, it's simply changing the shape.

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Quoted 2 lines

But it's very similar to what mempools are for.

As for dynamic caching or mempools. It requires extra logic on top of RCU to move things forward and it might be not efficient way. As a side effect, maintaining of the bulk arrays in the separate worker thread will introduce other drawbacks:

a) There is an extra latency window, a time during which a fallback mechanism is used until pages are obtained via the special worker for further pointers collecting over arrays.

b) It is impossible to predict how many pages will be required to cover a demand that is controlled by different workloads on various systems. It would require a rough value.

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Quoted 6 lines

Quoted 2 lines

But it's very similar to what mempools are for.

As for dynamic caching or mempools. It requires extra logic on top of RCU to move things forward and it might be not efficient way. As a side effect, maintaining of the bulk arrays in the separate worker thread will introduce other drawbacks:

This is true but it is also true that it is RCU to require this special logic and we can expect that we might need to fine tune this logic depending on the RCU usage. We definitely do not want to tune the generic page allocator for a very specific usecase, do we?

do we?

Quoted 3 lines

a) There is an extra latency window, a time during which a fallback mechanism is used until pages are obtained via the special worker for further pointers collecting over arrays.

This will be always the case for async refilling. More importantly this will be a bigger problem at the page allocator level which has other users other than RCU so more users are suffering...

Quoted 3 lines

b) It is impossible to predict how many pages will be required to cover a demand that is controlled by different workloads on various systems. It would require a rough value.

I have asked for some actual numbers for real life scenarios this work is meant to cover. There was nothing presented so far. We can hand wave for ever but this will not move us forward much. As I've said in other email, few dozens pages per CPU by default will hardly get noticeable. You have a trivial initial implementation and can build on top in incremental steps. You can kick a background allocator to start new allocations when the pool hits a watermark and aggressivelly remove cached pages when they are not used. You will have quite a freedom to fine tune the scheme which is much harder for the page allocator because there are many other consumers.

Anyway, I am afraid that we are going in circles here. We do not have any meaningful numbers to claim memory footprint problems. There is a clear opposition to hook into page allocator for reasons already mentioned. You are looking for a dedicated memory pool and it should be quite trivial to develop one and fine tune it for your specific usecase. All that on top of page allocator. Unless this is seen as completely unfeasible based on some solid arguments then we can start talking about the page allocator itself.

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Quoted 6 lines

Quoted 2 lines

But it's very similar to what mempools are for.

As for dynamic caching or mempools. It requires extra logic on top of RCU to move things forward and it might be not efficient way. As a side effect, maintaining of the bulk arrays in the separate worker thread will introduce other drawbacks:

Quoted 5 lines

This is true but it is also true that it is RCU to require this special logic and we can expect that we might need to fine tune this logic depending on the RCU usage. We definitely do not want to tune the generic page allocator for a very specific usecase, do we?

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

Quoted 24 lines

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h struct vm_area_struct; · line 246

Quoted 10 lines

include/linux/gfp.h +3 -1

@@ -238,7 +238,9 @@ struct vm_area_struct;
  * %__GFP_FOO flags as necessary.
  *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
- * watermark is applied to allow access to "atomic reserves"
+ * watermark is applied to allow access to "atomic reserves".
+ * The current implementation doesn't support NMI and other non-preemptive context
+ * (e.g. raw_spin_lock).
  *
  * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires
  * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim.

To me it is clear. But also above conflicting statement:

<snip>
%GFP_ATOMIC users can not sleep and need the allocation to succeed. A %lower <snip>

should be rephrased, IMHO.

Quoted 40 lines · Uladzislau Rezki

Quoted 24 lines

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h struct vm_area_struct; · line 246

Quoted 10 lines

include/linux/gfp.h +3 -1

@@ -238,7 +238,9 @@ struct vm_area_struct;
  * %__GFP_FOO flags as necessary.
  *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
- * watermark is applied to allow access to "atomic reserves"
+ * watermark is applied to allow access to "atomic reserves".
+ * The current implementation doesn't support NMI and other non-preemptive context
+ * (e.g. raw_spin_lock).
  *
  * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires
  * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim.

To me it is clear. But also above conflicting statement:

<snip>
%GFP_ATOMIC users can not sleep and need the allocation to succeed. A %lower <snip>

Quoted 1 line

should be rephrased, IMHO.

Any suggestions? Or more specifics about which part is conflicting? It tries to say that there is a higher demand to succeed even though the context cannot sleep to take active measures to achieve that. So the only way to achieve that is to break the watermakrs to a certain degree which is making them more "higher class" than other allocations.

Quoted 41 lines · Michal Hocko

Quoted 40 lines · Uladzislau Rezki

Quoted 24 lines

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h struct vm_area_struct; · line 246

Quoted 10 lines

include/linux/gfp.h +3 -1

@@ -238,7 +238,9 @@ struct vm_area_struct;
  * %__GFP_FOO flags as necessary.
  *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
- * watermark is applied to allow access to "atomic reserves"
+ * watermark is applied to allow access to "atomic reserves".
+ * The current implementation doesn't support NMI and other non-preemptive context
+ * (e.g. raw_spin_lock).
  *
  * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires
  * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim.

To me it is clear. But also above conflicting statement:

<snip>
%GFP_ATOMIC users can not sleep and need the allocation to succeed. A %lower <snip>

Quoted 8 lines

Quoted 1 line

should be rephrased, IMHO.

Any suggestions? Or more specifics about which part is conflicting? It tries to say that there is a higher demand to succeed even though the context cannot sleep to take active measures to achieve that. So the only way to achieve that is to break the watermakrs to a certain degree which is making them more "higher class" than other allocations.

Michal, i had only one concern about it. It says that %GFP_ATOMIC users can not sleep, i.e. callers know that they are in atomic, thus no any sleeping, but the chose they make will force them to sleep.

Quoted 50 lines · Uladzislau Rezki

Quoted 49 lines · Michal Hocko

Quoted 36 lines · Uladzislau Rezki

Quoted 24 lines

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h struct vm_area_struct; · line 246

Quoted 10 lines

include/linux/gfp.h +3 -1

@@ -238,7 +238,9 @@ struct vm_area_struct;
  * %__GFP_FOO flags as necessary.
  *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
- * watermark is applied to allow access to "atomic reserves"
+ * watermark is applied to allow access to "atomic reserves".
+ * The current implementation doesn't support NMI and other non-preemptive context
+ * (e.g. raw_spin_lock).
  *
  * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires
  * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim.

To me it is clear. But also above conflicting statement:

Quoted 5 lines

<snip>
%GFP_ATOMIC users can not sleep and need the allocation to succeed. A %lower <snip>

should be rephrased, IMHO.

Any suggestions? Or more specifics about which part is conflicting? It tries to say that there is a higher demand to succeed even though the context cannot sleep to take active measures to achieve that. So the only way to achieve that is to break the watermakrs to a certain degree which is making them more "higher class" than other allocations.

Quoted 3 lines

Michal, i had only one concern about it. It says that %GFP_ATOMIC users can not sleep, i.e. callers know that they are in atomic, thus no any sleeping, but the chose they make will force them to sleep.

I am not sure I follow you here. Do you mean they will be forced to sleep with PREEMPT_RT?

Quoted 51 lines · Michal Hocko

Quoted 50 lines · Uladzislau Rezki

Quoted 49 lines · Michal Hocko

Quoted 36 lines · Uladzislau Rezki

Quoted 24 lines

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h struct vm_area_struct; · line 246

Quoted 10 lines

include/linux/gfp.h +3 -1

@@ -238,7 +238,9 @@ struct vm_area_struct;
  * %__GFP_FOO flags as necessary.
  *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
- * watermark is applied to allow access to "atomic reserves"
+ * watermark is applied to allow access to "atomic reserves".
+ * The current implementation doesn't support NMI and other non-preemptive context
+ * (e.g. raw_spin_lock).
  *
  * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires
  * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim.

To me it is clear. But also above conflicting statement:

Quoted 5 lines

<snip>
%GFP_ATOMIC users can not sleep and need the allocation to succeed. A %lower <snip>

should be rephrased, IMHO.

Any suggestions? Or more specifics about which part is conflicting? It tries to say that there is a higher demand to succeed even though the context cannot sleep to take active measures to achieve that. So the only way to achieve that is to break the watermakrs to a certain degree which is making them more "higher class" than other allocations.

Quoted 7 lines

Quoted 3 lines

Michal, i had only one concern about it. It says that %GFP_ATOMIC users can not sleep, i.e. callers know that they are in atomic, thus no any sleeping, but the chose they make will force them to sleep.

I am not sure I follow you here. Do you mean they will be forced to sleep with PREEMPT_RT?

Exactly :)

Quoted 59 lines · Uladzislau Rezki

Quoted 58 lines · Michal Hocko

Quoted 50 lines · Uladzislau Rezki

Quoted 41 lines · Michal Hocko

Quoted 40 lines · Uladzislau Rezki

Quoted 24 lines

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h struct vm_area_struct; · line 246

Quoted 10 lines

include/linux/gfp.h +3 -1

@@ -238,7 +238,9 @@ struct vm_area_struct;
  * %__GFP_FOO flags as necessary.
  *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
- * watermark is applied to allow access to "atomic reserves"
+ * watermark is applied to allow access to "atomic reserves".
+ * The current implementation doesn't support NMI and other non-preemptive context
+ * (e.g. raw_spin_lock).
  *
  * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires
  * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim.

To me it is clear. But also above conflicting statement:

<snip>
%GFP_ATOMIC users can not sleep and need the allocation to succeed. A %lower <snip>

Quoted 8 lines

Quoted 1 line

should be rephrased, IMHO.

Any suggestions? Or more specifics about which part is conflicting? It tries to say that there is a higher demand to succeed even though the context cannot sleep to take active measures to achieve that. So the only way to achieve that is to break the watermakrs to a certain degree which is making them more "higher class" than other allocations.

Quoted 3 lines

Michal, i had only one concern about it. It says that %GFP_ATOMIC users can not sleep, i.e. callers know that they are in atomic, thus no any sleeping, but the chose they make will force them to sleep.

I am not sure I follow you here. Do you mean they will be forced to sleep with PREEMPT_RT?

Quoted 1 line

Exactly :)

We can make that more specific once RT patchset is merged. As of now this is not the thing in the Linus tree. I believe there will be more to clarify about atomic contexts in the RT tree as it means something else than people are used to think.

Quoted 20 lines · Uladzislau Rezki

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Quoted 6 lines

Quoted 2 lines

But it's very similar to what mempools are for.

As for dynamic caching or mempools. It requires extra logic on top of RCU to move things forward and it might be not efficient way. As a side effect, maintaining of the bulk arrays in the separate worker thread will introduce other drawbacks:

Quoted 5 lines

This is true but it is also true that it is RCU to require this special logic and we can expect that we might need to fine tune this logic depending on the RCU usage. We definitely do not want to tune the generic page allocator for a very specific usecase, do we?

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h +0 -0

  *
  * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires
  * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim.
 
[...]

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 44 lines · Michal Hocko

Quoted 20 lines · Uladzislau Rezki

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Quoted 6 lines

Quoted 2 lines

But it's very similar to what mempools are for.

As for dynamic caching or mempools. It requires extra logic on top of RCU to move things forward and it might be not efficient way. As a side effect, maintaining of the bulk arrays in the separate worker thread will introduce other drawbacks:

Quoted 5 lines

This is true but it is also true that it is RCU to require this special logic and we can expect that we might need to fine tune this logic depending on the RCU usage. We definitely do not want to tune the generic page allocator for a very specific usecase, do we?

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h struct vm_area_struct; · line 243

Quoted 7 lines

include/linux/gfp.h +3 -1

@@ -238,7 +238,9 @@ struct vm_area_struct;
  * %__GFP_FOO flags as necessary.
  *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
- * watermark is applied to allow access to "atomic reserves"
+ * watermark is applied to allow access to "atomic reserves".
+ * The current implementation doesn't support NMI and other non-preemptive context
+ * (e.g. raw_spin_lock).

I think documenting is useful.

Could it be more explicit in what the issue is? Something like:

* Even with GFP_ATOMIC, calls to the allocator can sleep on PREEMPT_RT systems. Therefore, the current low-level allocator implementation does not support being called from special contexts that are atomic on RT - such as NMI and raw_spin_lock. Due to these constraints and considering calling code usually has no control over the PREEMPT_RT configuration, callers of the allocator should avoid calling the allocator from these cotnexts even in non-RT systems.

Quoted 57 lines · Joel Fernandes

Quoted 44 lines · Michal Hocko

Quoted 20 lines · Uladzislau Rezki

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Quoted 6 lines

Quoted 2 lines

But it's very similar to what mempools are for.

As for dynamic caching or mempools. It requires extra logic on top of RCU to move things forward and it might be not efficient way. As a side effect, maintaining of the bulk arrays in the separate worker thread will introduce other drawbacks:

Quoted 5 lines

This is true but it is also true that it is RCU to require this special logic and we can expect that we might need to fine tune this logic depending on the RCU usage. We definitely do not want to tune the generic page allocator for a very specific usecase, do we?

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h struct vm_area_struct; · line 243

Quoted 7 lines

include/linux/gfp.h +3 -1

@@ -238,7 +238,9 @@ struct vm_area_struct;
  * %__GFP_FOO flags as necessary.
  *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
- * watermark is applied to allow access to "atomic reserves"
+ * watermark is applied to allow access to "atomic reserves".
+ * The current implementation doesn't support NMI and other non-preemptive context
+ * (e.g. raw_spin_lock).

I think documenting is useful.

Could it be more explicit in what the issue is? Something like:

Quoted 7 lines

* Even with GFP_ATOMIC, calls to the allocator can sleep on PREEMPT_RT systems. Therefore, the current low-level allocator implementation does not support being called from special contexts that are atomic on RT - such as NMI and raw_spin_lock. Due to these constraints and considering calling code usually has no control over the PREEMPT_RT configuration, callers of the allocator should avoid calling the allocator from these cotnexts even in non-RT systems.

I do not mind documenting RT specific behavior but as mentioned in other reply, this should likely go via RT tree for now. There is likely more to clarify about atomicity for PREEMPT_RT.

Quoted 67 lines · Michal Hocko

Quoted 64 lines · Joel Fernandes

Quoted 44 lines · Michal Hocko

Quoted 20 lines · Uladzislau Rezki

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Quoted 6 lines

Quoted 2 lines

But it's very similar to what mempools are for.

As for dynamic caching or mempools. It requires extra logic on top of RCU to move things forward and it might be not efficient way. As a side effect, maintaining of the bulk arrays in the separate worker thread will introduce other drawbacks:

Quoted 5 lines

This is true but it is also true that it is RCU to require this special logic and we can expect that we might need to fine tune this logic depending on the RCU usage. We definitely do not want to tune the generic page allocator for a very specific usecase, do we?

Quoted 6 lines

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Yes this sucks. But this is something that we likely really want to live with. We have to explicitly _document_ that really atomic contexts in RT cannot use the allocator. From the past discussions we've had this is likely the most reasonable way forward because we do not really want to encourage anybody to do something like that and there should be ways around that. The same is btw. true also for !RT. The allocator is not NMI safe and while we should be able to make it compatible I am not convinced we really want to.

Would something like this be helpful wrt documentation?

include/linux/gfp.h +0 -0

index 67a0774e080b..9fcd47606493 100644

include/linux/gfp.h struct vm_area_struct; · line 243

Quoted 7 lines

include/linux/gfp.h +3 -1

@@ -238,7 +238,9 @@ struct vm_area_struct;
  * %__GFP_FOO flags as necessary.
  *
  * %GFP_ATOMIC users can not sleep and need the allocation to succeed. A lower
- * watermark is applied to allow access to "atomic reserves"
+ * watermark is applied to allow access to "atomic reserves".
+ * The current implementation doesn't support NMI and other non-preemptive context
+ * (e.g. raw_spin_lock).

I think documenting is useful.

Could it be more explicit in what the issue is? Something like:

* Even with GFP_ATOMIC, calls to the allocator can sleep on PREEMPT_RT systems. Therefore, the current low-level allocator implementation does not support being called from special contexts that are atomic on RT - such as NMI and raw_spin_lock. Due to these constraints and considering calling code usually has no control over the PREEMPT_RT configuration, callers of the allocator should avoid calling the allocator from these cotnexts even in non-RT systems.

Quoted 3 lines

I do not mind documenting RT specific behavior but as mentioned in other reply, this should likely go via RT tree for now. There is likely more to clarify about atomicity for PREEMPT_RT.

I am sorry, I did not understand what you meant by something missing in Linus Tree. CONFIG_PREEMPT_RT is there.

Could you clarify? Also the CONFIG_PREEMPT_RT is the only thing driving this requirement for GFP_ATOMIC right? Or are there non-RT reasons why GFP_ATOMIC allocation cannot be done from NMI/raw_spin_lock ?

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

I think documenting is useful.

Could it be more explicit in what the issue is? Something like:

* Even with GFP_ATOMIC, calls to the allocator can sleep on PREEMPT_RT systems. Therefore, the current low-level allocator implementation does not support being called from special contexts that are atomic on RT - such as NMI and raw_spin_lock. Due to these constraints and considering calling code usually has no control over the PREEMPT_RT configuration, callers of the allocator should avoid calling the allocator from these cotnexts even in non-RT systems.

Quoted 6 lines

Quoted 3 lines

I do not mind documenting RT specific behavior but as mentioned in other reply, this should likely go via RT tree for now. There is likely more to clarify about atomicity for PREEMPT_RT.

I am sorry, I did not understand what you meant by something missing in Linus Tree. CONFIG_PREEMPT_RT is there.

OK, I was not aware we already CONFIG_PREEMPT_RT in the three. There is still a lot from the RT patchset including sleeping spin locks that make a real difference. Or maybe I haven't checked properly.

Quoted 4 lines

Could you clarify? Also the CONFIG_PREEMPT_RT is the only thing driving this requirement for GFP_ATOMIC right? Or are there non-RT reasons why GFP_ATOMIC allocation cannot be done from NMI/raw_spin_lock ?

I have already sent a clarification patch [1]. NMI is not supported regardless of the preemption mode. Hope this clarifies it a bit.

[1] http://lkml.kernel.org/r/[email protected]

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Quoted 6 lines

Quoted 2 lines

But it's very similar to what mempools are for.

As for dynamic caching or mempools. It requires extra logic on top of RCU to move things forward and it might be not efficient way. As a side effect, maintaining of the bulk arrays in the separate worker thread will introduce other drawbacks:

Quoted 5 lines

This is true but it is also true that it is RCU to require this special logic and we can expect that we might need to fine tune this logic depending on the RCU usage. We definitely do not want to tune the generic page allocator for a very specific usecase, do we?

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Do you agree with it?

Quoted 25 lines · Uladzislau Rezki

Quoted 15 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

All good points!

On the other hand, duplicating a portion of the allocator functionality within RCU increases the amount of reserved memory, and needlessly most of the time.

Quoted 6 lines

Quoted 2 lines

But it's very similar to what mempools are for.

As for dynamic caching or mempools. It requires extra logic on top of RCU to move things forward and it might be not efficient way. As a side effect, maintaining of the bulk arrays in the separate worker thread will introduce other drawbacks:

Quoted 5 lines

This is true but it is also true that it is RCU to require this special logic and we can expect that we might need to fine tune this logic depending on the RCU usage. We definitely do not want to tune the generic page allocator for a very specific usecase, do we?

I look at it in scope of GFP_ATOMIC/GFP_NOWAIT issues, i.e. inability to provide a memory service for contexts which are not allowed to sleep, RCU is part of them. Both flags used to provide such ability before but not anymore.

Quoted 2 lines

Do you agree with it?

I was led to believe that hte problem was taking the zone lock while holding a raw spinlock that was specific to RCU.

Are you saying that GFP_ATOMIC/GFP_NOWAIT users are also holding raw spinlocks at the same time on RT?

[Cc Mel - the thread starts http://lkml.kernel.org/r/[email protected]]

Quoted 14 lines · Uladzislau Rezki

Hello, Michal.

Quoted 7 lines

Quoted 6 lines

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

Quoted 5 lines

No worries I do not feel like a whipping boy here. But do expect me to argue against the approach. I would also appreciate it if there was some more information on other attempts, why they have failed. E.g. why pre-allocation is not an option that works well enough in most reasonable workloads.

Quoted 5 lines

Pre-allocating has some drawbacks:

a) It is impossible to predict how many pages will be required to cover a demand that is controlled by different workloads on various systems.

Yes, this is not trivial but not a rocket science either. Remember that you are relying on a very dumb watermark based pcp pool from the allocator. Mimicing a similar implementation shouldn't be all that hard and you will get your own pool which doesn't affect other page allocator users as much as a bonus.

Quoted 2 lines

b) Memory overhead since we do not know how much pages should be
preloaded: 100, 200 or 300

Does anybody who really needs this optimization actually cares about 300 pages?

Quoted 4 lines

As for memory overhead, it is important to reduce it because of embedded devices like phones, where a low memory condition is a big issue. In that sense pre-allocating is something that we strongly would like to avoid.

How big "machines" are we talking about here? I would expect that really tiny machines would have hard times to really fill up thousands of pages with pointers to free...

Would a similar scaling as the page allocator feasible. Really I mostly do care about shared nature of the pcp allocator list that one user can easily monopolize with this API.

Quoted 7 lines

Quoted 3 lines

I would also appreciate some more thoughts why we
need to optimize for heavy abusers of RCU (like close(open) extremes).

I think here is a small misunderstanding. Please note, that is not only about performance and corner cases. There is a single argument support of the kvfree_rcu(ptr), where maintaining an array in time is needed. The fallback of the single argument case is extrimely slow.

This should be part of the changelog.

Quoted 2 lines

Single-argument details is here: https://lkml.org/lkml/2020/4/28/1626

Error 501

Quoted 16 lines

Quoted 16 lines

Quoted 7 lines

Quoted 5 lines

I strongly agree with Thomas http://lkml.kernel.org/r/[email protected]
that this optimization is not aiming at reasonable workloads. Really, go
with pre-allocated buffer and fallback to whatever slow path you have already. Exposing more internals of the allocator is not going to do any
good for long term maintainability.

I suggest that you carefully re-read the thread following that email.

I clearly remember Thomas not being particularly happy that you optimize for a corner case. I do not remember there being a consensus that this is the right approach. There was some consensus that this is better than a gfp flag. Still quite bad though if you ask me.

Quoted 2 lines

Given a choice between making users unhappy and making developers unhappy, I will side with the users each and every time.

Quoted 8 lines

Quoted 5 lines

Well, let me rephrase. It is not only about me (as a developer) being unhappy but also all the side effects this would have for users when performance of their favorite workload declines for no apparent reason just because pcp caches are depleted by an unrelated process.

If depleted, we have a special worker that charge it. From the other hand, the pcplist can be depleted by its nature, what _is_ not wrong. But just in case we secure it since you had a concern about it.

pcp free lists should ever get empty when we run out of memory and need to reclaim. Otherwise they are constantly refilled/rebalanced on demand. The fact that you are refilling them from outside just suggest that you are operating on a wrong layer. Really, create your own pool of pages and rebalance them based on the workload.

Quoted 1 line

Could you please specify a real test case or workload you are talking about?

I am not a performance expert but essentially any memory allocator heavy workload might notice. I am pretty sure Mel would tell you more.

Signature · 2 lines

Michal Hocko
SUSE Labs

Quoted 15 lines · Paul E. McKenney

Quoted 7 lines · Michal Hocko

On Fri 18-09-20 21:48:15, Uladzislau Rezki (Sony) wrote:
[...]

Quoted 4 lines

Proposal
========
Introduce a lock-free function that obtain a page from the per-cpu-lists on current CPU. It returns NULL rather than acquiring any non-raw spinlock.

Quoted 6 lines

I was not happy about this solution when we have discussed this last time and I have to say I am still not happy. This is exposing an internal allocator optimization and allows a hard to estimate consumption of pcp free pages. IIUC this run on pcp cache can be controled directly from userspace (close(open) loop IIRC) which makes it even bigger no-no.

Quoted 5 lines

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

No worries I do not feel like a whipping boy here. But do expect me to argue against the approach. I would also appreciate it if there was some more information on other attempts, why they have failed. E.g. why pre-allocation is not an option that works well enough in most reasonable workloads. I would also appreciate some more thoughts why we need to optimize for heavy abusers of RCU (like close(open) extremes).

Quoted 6 lines

Quoted 5 lines

I strongly agree with Thomas http://lkml.kernel.org/r/[email protected]
that this optimization is not aiming at reasonable workloads. Really, go
with pre-allocated buffer and fallback to whatever slow path you have already. Exposing more internals of the allocator is not going to do any
good for long term maintainability.

Quoted 1 line

I suggest that you carefully re-read the thread following that email.

I clearly remember Thomas not being particularly happy that you optimize for a corner case. I do not remember there being a consensus that this is the right approach. There was some consensus that this is better than a gfp flag. Still quite bad though if you ask me.

Quoted 2 lines

Given a choice between making users unhappy and making developers unhappy, I will side with the users each and every time.

Well, let me rephrase. It is not only about me (as a developer) being unhappy but also all the side effects this would have for users when performance of their favorite workload declines for no apparent reason just because pcp caches are depleted by an unrelated process.

Quoted 22 lines · Michal Hocko

Quoted 15 lines · Paul E. McKenney

Quoted 7 lines · Michal Hocko

On Fri 18-09-20 21:48:15, Uladzislau Rezki (Sony) wrote:
[...]

Quoted 4 lines

Proposal
========
Introduce a lock-free function that obtain a page from the per-cpu-lists on current CPU. It returns NULL rather than acquiring any non-raw spinlock.

Quoted 6 lines

I was not happy about this solution when we have discussed this last time and I have to say I am still not happy. This is exposing an internal allocator optimization and allows a hard to estimate consumption of pcp free pages. IIUC this run on pcp cache can be controled directly from userspace (close(open) loop IIRC) which makes it even bigger no-no.

Quoted 5 lines

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

Quoted 6 lines

No worries I do not feel like a whipping boy here. But do expect me to argue against the approach. I would also appreciate it if there was some more information on other attempts, why they have failed. E.g. why pre-allocation is not an option that works well enough in most reasonable workloads. I would also appreciate some more thoughts why we need to optimize for heavy abusers of RCU (like close(open) extremes).

Not optimizing for them, but rather defending against them. Uladzislau gave the example of low-memory phones. And we have quite the array of defenses against other userspace bugs including MMUs, the "limit" command, and so on.

There have been quite a few successful attempts, starting from the introduction of blimit and RCU-bh more than 15 years ago, continuing through making call_rcu() jump-start grace periods, IPIing reluctant CPUs, tuning RCU callback offloading, and many others. But these prior approaches have only taken us so far.

Other approaches under consideration include making CONFIG_PREEMPT_COUNT unconditional and thus allowing call_rcu() and kvfree_rcu() to determine whether direct calls to the allocator are safe (some guy named Linus doesn't like this one),

preallocation (Uladzislau covered this, and
the amount that would need to be preallocated is excessive), deferring allocation to RCU_SOFTIRQ (this would also need CONFIG_PREEMPT_COUNT), and deferring to some clean context (which is the best we can do within the confines of RCU, but which can have issues with delay).

So it is not the need to address this general problem that is new. Far from it! What is new is the need for changes outside of RCU.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

I strongly agree with Thomas http://lkml.kernel.org/r/[email protected]
that this optimization is not aiming at reasonable workloads. Really, go
with pre-allocated buffer and fallback to whatever slow path you have already. Exposing more internals of the allocator is not going to do any
good for long term maintainability.

Quoted 6 lines

Quoted 1 line

I suggest that you carefully re-read the thread following that email.

I clearly remember Thomas not being particularly happy that you optimize for a corner case. I do not remember there being a consensus that this is the right approach. There was some consensus that this is better than a gfp flag. Still quite bad though if you ask me.

Again, this "optimization" is for robustness more than raw speed.

Quoted 3 lines

Quoted 2 lines

Given a choice between making users unhappy and making developers unhappy, I will side with the users each and every time.

Quoted 4 lines

Well, let me rephrase. It is not only about me (as a developer) being unhappy but also all the side effects this would have for users when performance of their favorite workload declines for no apparent reason just because pcp caches are depleted by an unrelated process.

But in the close(open()) case, wouldn't the allocations on the open() side refill those caches?

Quoted 35 lines · Paul E. McKenney

Quoted 22 lines · Michal Hocko

Quoted 15 lines · Paul E. McKenney

Quoted 7 lines · Michal Hocko

On Fri 18-09-20 21:48:15, Uladzislau Rezki (Sony) wrote:
[...]

Quoted 4 lines

Proposal
========
Introduce a lock-free function that obtain a page from the per-cpu-lists on current CPU. It returns NULL rather than acquiring any non-raw spinlock.

Quoted 6 lines

I was not happy about this solution when we have discussed this last time and I have to say I am still not happy. This is exposing an internal allocator optimization and allows a hard to estimate consumption of pcp free pages. IIUC this run on pcp cache can be controled directly from userspace (close(open) loop IIRC) which makes it even bigger no-no.

Quoted 5 lines

Yes, I do well remember that you are unhappy with this approach. Unfortunately, thus far, there is no solution that makes all developers happy. You might be glad to hear that we are also looking into other solutions, each of which makes some other developers unhappy. So we are at least not picking on you alone. :-/

Quoted 6 lines

No worries I do not feel like a whipping boy here. But do expect me to argue against the approach. I would also appreciate it if there was some more information on other attempts, why they have failed. E.g. why pre-allocation is not an option that works well enough in most reasonable workloads. I would also appreciate some more thoughts why we need to optimize for heavy abusers of RCU (like close(open) extremes).

Not optimizing for them, but rather defending against them. Uladzislau gave the example of low-memory phones. And we have quite the array of defenses against other userspace bugs including MMUs, the "limit" command, and so on.

Quoted 5 lines

There have been quite a few successful attempts, starting from the introduction of blimit and RCU-bh more than 15 years ago, continuing through making call_rcu() jump-start grace periods, IPIing reluctant CPUs, tuning RCU callback offloading, and many others. But these prior approaches have only taken us so far.

Thank you this is useful. It gives me some idea about the problem but I still cannot picture how serious the problem really is. Is this a DoS like scenario where an unprivileged task is able to monopolize the system/CPU to do all the RCU heavy lifting? Are other RCU users deprived from doing their portion? How much expediting helps? Does it really solve the problem or it merely shifts it around as you will have hard time to keep up with more and more tasks to hit the same path and the pre-allocated memory is a very finite resource.

Quoted 4 lines

Other approaches under consideration include making CONFIG_PREEMPT_COUNT unconditional and thus allowing call_rcu() and kvfree_rcu() to determine whether direct calls to the allocator are safe (some guy named Linus doesn't like this one),

I assume that the primary argument is the overhead, right? Do you happen to have any reference?

Quoted 5 lines

preallocation (Uladzislau covered this, and
the amount that would need to be preallocated is excessive), deferring allocation to RCU_SOFTIRQ (this would also need CONFIG_PREEMPT_COUNT), and deferring to some clean context (which is the best we can do within the confines of RCU, but which can have issues with delay).

I have already replied to that so let's not split the discussion into several fronts.

Quoted 26 lines

So it is not the need to address this general problem that is new. Far from it! What is new is the need for changes outside of RCU.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

I strongly agree with Thomas http://lkml.kernel.org/r/[email protected]
that this optimization is not aiming at reasonable workloads. Really, go
with pre-allocated buffer and fallback to whatever slow path you have already. Exposing more internals of the allocator is not going to do any
good for long term maintainability.

Quoted 6 lines

Quoted 1 line

I suggest that you carefully re-read the thread following that email.

I clearly remember Thomas not being particularly happy that you optimize for a corner case. I do not remember there being a consensus that this is the right approach. There was some consensus that this is better than a gfp flag. Still quite bad though if you ask me.

Again, this "optimization" is for robustness more than raw speed.

Quoted 7 lines

Quoted 2 lines

Given a choice between making users unhappy and making developers unhappy, I will side with the users each and every time.

Well, let me rephrase. It is not only about me (as a developer) being unhappy but also all the side effects this would have for users when performance of their favorite workload declines for no apparent reason just because pcp caches are depleted by an unrelated process.

Quoted 2 lines

But in the close(open()) case, wouldn't the allocations on the open() side refill those caches?

Yes pcp lists will eventually get replenished. This is not a correctness problem I am pointing out. It is the fact that any user of this new interface can monopolize the _global_ pool which would affect all other users of the pool.

Quoted 5 lines

Quoted 4 lines

Other approaches under consideration include making CONFIG_PREEMPT_COUNT unconditional and thus allowing call_rcu() and kvfree_rcu() to determine whether direct calls to the allocator are safe (some guy named Linus doesn't like this one),

Quoted 2 lines

I assume that the primary argument is the overhead, right? Do you happen to have any reference?

Jon Corbet wrote a very nice article summarizing the current situation:
https://lwn.net/Articles/831678/. Thomas's measurements show no visible system-level performance impact. I will let Uladzislau present his more microbenchmarky performance work.

Some background and kfree_rcu()
===============================
The pointers to be freed are stored in the per-cpu array to improve performance, to enable an easier-to-use API, to accommodate vmalloc memmory and to support a single argument of the kfree_rcu() when only a pointer is passed. More details are below.

In order to maintain such per-CPU arrays there is a need in dynamic allocation when a current array is fully populated and a new block is required. See below the example:

0 1 2 3 0 1 2 3
|p|p|p|p| -> |p|p|p|p| -> NULL

there are two pointer-blocks, each one can store 4 addresses which will be freed after a grace period is passed. In reality we store PAGE_SIZE / sizeof(void *). So to maintain such blocks a single page is obtain via the page allocator:

bnode = (struct kvfree_rcu_bulk_data *)
__get_free_page(GFP_NOWAIT | __GFP_NOWARN);

after that it is attached to the "head" and its "next" pointer is set to previous "head", so the list of blocks can be maintained and grow dynamically until it gets drained by the reclaiming thread.

Please note. There is always a fallback if an allocation fails. In the single argument, this is a call to synchronize_rcu() and for the two arguments case this is to use rcu_head structure embedded in the object being free, and then paying cache-miss penalty, also invoke the kfree() per object instead of kfree_bulk() for groups of objects.

Why we maintain arrays/blocks instead of linking objects by the regular "struct rcu_head" technique. See below a few but main reasons:

a) A memory can be reclaimed by invoking of the kfree_bulk() interface that requires passing an array and number of entries in it. That reduces the per-object overhead caused by calling kfree() per-object. This reduces the reclamation time.

b) Improves locality and reduces the number of cache-misses, due to "pointer chasing" between objects, which can be far spread between each other.

c) Support a "single argument" in the kvfree_rcu() void *ptr = kvmalloc(some_bytes, GFP_KERNEL); if (ptr)

        kvfree_rcu(ptr);

We need it when an "rcu_head" is not embed into a stucture but an object must be freed after a grace period. Therefore for the single argument, such objects cannot be queued on a linked list.

   So nowadays, since we do not have a single argument but we see the
   demand in it, to workaround it people just do a simple not efficient
   sequence:
   <snip>
       synchronize_rcu(); /* Can be long and blocks a current context */
       kfree(p);
   <snip>

   More details is here: https://lkml.org/lkml/2020/4/28/1626

d) To distinguish vmalloc pointers between SLAB ones. It becomes possible to invoke the right freeing API for the right kind of pointer, kfree_bulk() or TBD: vmalloc_bulk().

e) Speeding up the post-grace-period freeing reduces the chance of a flood of callback's OOMing the system.

Also, please have a look here: https://lkml.org/lkml/2020/7/30/1166

Proposal
========
Introduce a lock-free function that obtain a page from the per-cpu-lists on current CPU. It returns NULL rather than acquiring any non-raw spinlock.

Description
===========
The page allocator has two phases, fast path and slow one. We are interested in fast path and order-0 allocations. In its turn it is divided also into two
phases: lock-less and not:

1) As a first step the page allocator tries to obtain a page from the per-cpu-list, so each CPU has its own one. That is why this step is lock-less and fast. Basically it disables irqs on current CPU in order to access to per-cpu data and remove a first element from the pcp-list. An element/page is returned to an user.

2) If there is no any available page in per-cpu-list, the second step is involved. It removes a specified number of elements from the buddy allocator transferring them to the "supplied-list/per-cpu-list" described in [1].

Summarizing. The __rcu_alloc_page_lockless() covers only [1] and can not do step [2], due to the fact that [2] requires an access to zone->lock. It implies that it is super fast, but a higher rate of fails is also expected.

Usage: __rcu_alloc_page_lockless();

Link https://lore.kernel.org/lkml/[email protected]/ open here

Not-signed-off-by Peter Zijlstra <[email protected]>

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>

Quoted 99 lines · Uladzislau Rezki (Sony)

Some background and kfree_rcu()
===============================
The pointers to be freed are stored in the per-cpu array to improve performance, to enable an easier-to-use API, to accommodate vmalloc memmory and to support a single argument of the kfree_rcu() when only a pointer is passed. More details are below.

In order to maintain such per-CPU arrays there is a need in dynamic allocation when a current array is fully populated and a new block is required. See below the example:

0 1 2 3 0 1 2 3
|p|p|p|p| -> |p|p|p|p| -> NULL

there are two pointer-blocks, each one can store 4 addresses which will be freed after a grace period is passed. In reality we store PAGE_SIZE / sizeof(void *). So to maintain such blocks a single page is obtain via the page allocator:

bnode = (struct kvfree_rcu_bulk_data *)
__get_free_page(GFP_NOWAIT | __GFP_NOWARN);

after that it is attached to the "head" and its "next" pointer is set to previous "head", so the list of blocks can be maintained and grow dynamically until it gets drained by the reclaiming thread.

Please note. There is always a fallback if an allocation fails. In the single argument, this is a call to synchronize_rcu() and for the two arguments case this is to use rcu_head structure embedded in the object being free, and then paying cache-miss penalty, also invoke the kfree() per object instead of kfree_bulk() for groups of objects.

Why we maintain arrays/blocks instead of linking objects by the regular "struct rcu_head" technique. See below a few but main reasons:

a) A memory can be reclaimed by invoking of the kfree_bulk() interface that requires passing an array and number of entries in it. That reduces the per-object overhead caused by calling kfree() per-object. This reduces the reclamation time.

b) Improves locality and reduces the number of cache-misses, due to "pointer chasing" between objects, which can be far spread between each other.

c) Support a "single argument" in the kvfree_rcu() void *ptr = kvmalloc(some_bytes, GFP_KERNEL); if (ptr)

        kvfree_rcu(ptr);

We need it when an "rcu_head" is not embed into a stucture but an object must be freed after a grace period. Therefore for the single argument, such objects cannot be queued on a linked list.

   So nowadays, since we do not have a single argument but we see the
   demand in it, to workaround it people just do a simple not efficient
   sequence:
   <snip>
       synchronize_rcu(); /* Can be long and blocks a current context */
       kfree(p);
   <snip>

   More details is here: https://lkml.org/lkml/2020/4/28/1626

d) To distinguish vmalloc pointers between SLAB ones. It becomes possible to invoke the right freeing API for the right kind of pointer, kfree_bulk() or TBD: vmalloc_bulk().

e) Speeding up the post-grace-period freeing reduces the chance of a flood of callback's OOMing the system.

Also, please have a look here: https://lkml.org/lkml/2020/7/30/1166

Proposal
========
Introduce a lock-free function that obtain a page from the per-cpu-lists on current CPU. It returns NULL rather than acquiring any non-raw spinlock.

Description
===========
The page allocator has two phases, fast path and slow one. We are interested in fast path and order-0 allocations. In its turn it is divided also into two
phases: lock-less and not:

1) As a first step the page allocator tries to obtain a page from the per-cpu-list, so each CPU has its own one. That is why this step is lock-less and fast. Basically it disables irqs on current CPU in order to access to per-cpu data and remove a first element from the pcp-list. An element/page is returned to an user.

2) If there is no any available page in per-cpu-list, the second step is involved. It removes a specified number of elements from the buddy allocator transferring them to the "supplied-list/per-cpu-list" described in [1].

Summarizing. The __rcu_alloc_page_lockless() covers only [1] and can not do step [2], due to the fact that [2] requires an access to zone->lock. It implies that it is super fast, but a higher rate of fails is also expected.

Quoted 5 lines

Usage: __rcu_alloc_page_lockless();

Link https://lore.kernel.org/lkml/[email protected]/ open here

Not-signed-off-by Peter Zijlstra <[email protected]>

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>

After reading all the threads and mulling over this, I am going to deflect from Mel and Michal and not oppose the idea of lockless allocation. I would even prefer to do it via the gfp flag and not a completely separate path. Not using the exact code from v1, I think it could be done in a way that we don't actually look at the new flag until we find that pcplist is empty - which should not introduce overhead to the fast-fast path when pcpclist is not empty. It's more maintainable that adding new entry points, IMHO.

But there's still the condition that it's sufficiently shown that the allocation is useful for RCU. In that case I prefer that the page allocator (or MM in general) can give its users what they need without having to work around it.

Seems like GFP_ATOMIC is insufficient today so if that means we need a new flag for the raw spinlock context, so be it. But if your usage of __GFP_NO_LOCKS depends on the result of preempt_count() or similar check whether this is a context that needs it, I'd prefer to keep this in the caller.

<snip>
bool is_pcp_cache_empty(gfp_t gfp)

{
    struct per_cpu_pages *pcp;
    struct zoneref *ref;
    unsigned long flags;
    bool empty;

    ref = first_zones_zonelist(node_zonelist(
            numa_node_id(), gfp), gfp_zone(gfp), NULL);
    if (!ref->zone)
            return true;

    local_irq_save(flags);
    pcp = &this_cpu_ptr(ref->zone->pageset)->pcp;
    empty = list_empty(&pcp->lists[gfp_migratetype(gfp)]);
    local_irq_restore(flags);

    return empty;
}

disable_irq();
if (!is_pcp_cache_empty(GFP_NOWAIT))

    __get_free_page(GFP_NOWAIT);

enable_irq();
<snip>

Do you mean to have something like above? I mean some extra API function that returns true or false if fast-fast allocation can either occur or not. Above code works just fine and never touches main zone->lock.

On Wed 30-09-20 00:07:42, Uladzislau Rezki wrote:
[...]

Quoted 23 lines

<snip>
bool is_pcp_cache_empty(gfp_t gfp)

{
    struct per_cpu_pages *pcp;
    struct zoneref *ref;
    unsigned long flags;
    bool empty;

    ref = first_zones_zonelist(node_zonelist(
            numa_node_id(), gfp), gfp_zone(gfp), NULL);
    if (!ref->zone)
            return true;

    local_irq_save(flags);
    pcp = &this_cpu_ptr(ref->zone->pageset)->pcp;
    empty = list_empty(&pcp->lists[gfp_migratetype(gfp)]);
    local_irq_restore(flags);

    return empty;
}

disable_irq();
if (!is_pcp_cache_empty(GFP_NOWAIT))

Quoted 8 lines

    __get_free_page(GFP_NOWAIT);

enable_irq();
<snip>

Do you mean to have something like above? I mean some extra API function that returns true or false if fast-fast allocation can either occur or not. Above code works just fine and never touches main zone->lock.

The above code works with the _current_ implementation and it restricts its implementation to some degree. Future changes might get harder to implement with a pattern like this. I do not think we want users to be aware of internal implementation details like pcp caches, migrate types or others. While pcp caches are here for years and unlikely to change in a foreseeable future many details are changing on regular basis.

Quoted 10 lines · Vlastimil Babka

On 9/18/20 9:48 PM, Uladzislau Rezki (Sony) wrote:

After reading all the threads and mulling over this, I am going to deflect from Mel and Michal and not oppose the idea of lockless allocation. I would even prefer to do it via the gfp flag and not a completely separate path. Not using the exact code from v1, I think it could be done in a way that we don't actually look at the new flag until we find that pcplist is empty - which should not introduce overhead to the fast-fast path when pcpclist is not empty. It's more maintainable that adding new entry points, IMHO.

Thanks for reading all that from the beginning! It must be tough due to the fact there were lot of messages in the threads, so at least i was lost.

I agree that adding a new entry or separate lock-less function can be considered as something that is hard to maintain. I have a question here. I mean about your different look at it:

<snip>
bool is_pcp_cache_empty(gfp_t gfp)

{
    struct per_cpu_pages *pcp;
    struct zoneref *ref;
    unsigned long flags;
    bool empty;

    ref = first_zones_zonelist(node_zonelist(
            numa_node_id(), gfp), gfp_zone(gfp), NULL);
    if (!ref->zone)
            return true;

    local_irq_save(flags);
    pcp = &this_cpu_ptr(ref->zone->pageset)->pcp;
    empty = list_empty(&pcp->lists[gfp_migratetype(gfp)]);
    local_irq_restore(flags);

    return empty;
}

disable_irq();
if (!is_pcp_cache_empty(GFP_NOWAIT))

    __get_free_page(GFP_NOWAIT);

enable_irq();
<snip>

Do you mean to have something like above? I mean some extra API function that returns true or false if fast-fast allocation can either occur or not. Above code works just fine and never touches main zone->lock.

i.e. Instead of introducing an extra GFP_LOCKLESS flag or any new extra lock-less function. We could have something that checks a pcp page cache list, thus it can guarantee that a request would be accomplish using fast-fast path.

Quoted 50 lines · Uladzislau Rezki

Quoted 10 lines · Vlastimil Babka

On 9/18/20 9:48 PM, Uladzislau Rezki (Sony) wrote:

After reading all the threads and mulling over this, I am going to deflect from Mel and Michal and not oppose the idea of lockless allocation. I would even prefer to do it via the gfp flag and not a completely separate path. Not using the exact code from v1, I think it could be done in a way that we don't actually look at the new flag until we find that pcplist is empty - which should not introduce overhead to the fast-fast path when pcpclist is not empty. It's more maintainable that adding new entry points, IMHO.

Thanks for reading all that from the beginning! It must be tough due to the fact there were lot of messages in the threads, so at least i was lost.

I agree that adding a new entry or separate lock-less function can be considered as something that is hard to maintain. I have a question here. I mean about your different look at it:

<snip>
bool is_pcp_cache_empty(gfp_t gfp)

{
    struct per_cpu_pages *pcp;
    struct zoneref *ref;
    unsigned long flags;
    bool empty;

    ref = first_zones_zonelist(node_zonelist(
            numa_node_id(), gfp), gfp_zone(gfp), NULL);
    if (!ref->zone)
            return true;

    local_irq_save(flags);
    pcp = &this_cpu_ptr(ref->zone->pageset)->pcp;
    empty = list_empty(&pcp->lists[gfp_migratetype(gfp)]);
    local_irq_restore(flags);

    return empty;
}

disable_irq();
if (!is_pcp_cache_empty(GFP_NOWAIT))

    __get_free_page(GFP_NOWAIT);

enable_irq();
<snip>

Do you mean to have something like above? I mean some extra API function that returns true or false if fast-fast allocation can either occur or not. Above code works just fine and never touches main zone->lock.

Quoted 4 lines

i.e. Instead of introducing an extra GFP_LOCKLESS flag or any new extra lock-less function. We could have something that checks a pcp page cache list, thus it can guarantee that a request would be accomplish using fast-fast path.

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

Quoted 11 lines · Uladzislau Rezki

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

include/linux/gfp.h struct vm_area_struct; · line 42

Quoted 10 lines

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +1 -0

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Uladzislau Rezki

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

Quoted 14 lines

include/linux/gfp.h struct vm_area_struct; · line 42

Quoted 10 lines

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +1 -0

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

That is definitely true. I'm not happy with the GFP flag at all, the comment is at best a damage limiting move. It still would be better for a memory pool to be reserved and sized for critical allocations.

Quoted 2 lines

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

That would deserve to be taken to a dumpster and set on fire. The flag combination could be checked in the allocator but the allocator path fast paths are bad enough already.

Quoted 28 lines · Mel Gorman

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Uladzislau Rezki

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

Quoted 14 lines

include/linux/gfp.h struct vm_area_struct; · line 42

Quoted 10 lines

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +1 -0

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

Quoted 3 lines

That is definitely true. I'm not happy with the GFP flag at all, the comment is at best a damage limiting move. It still would be better for a memory pool to be reserved and sized for critical allocations.

Completely agreed. The only existing usecase is so special cased that a dedicated pool is not only easier to maintain but it should be also much better tuned for the specific workload. Something not really feasible with the allocator.

Quoted 3 lines

Quoted 2 lines

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

Quoted 3 lines

That would deserve to be taken to a dumpster and set on fire. The flag combination could be checked in the allocator but the allocator path fast paths are bad enough already.

If a new allocation/gfp mode is absolutely necessary then I believe that the most reasoanble way forward would be
#define GFP_NO_LOCK ((__force gfp_t)0)

Quoted 45 lines · Michal Hocko

Quoted 28 lines · Mel Gorman

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Uladzislau Rezki

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

Quoted 14 lines

include/linux/gfp.h struct vm_area_struct; · line 42

Quoted 10 lines

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +1 -0

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

Quoted 3 lines

That is definitely true. I'm not happy with the GFP flag at all, the comment is at best a damage limiting move. It still would be better for a memory pool to be reserved and sized for critical allocations.

Completely agreed. The only existing usecase is so special cased that a dedicated pool is not only easier to maintain but it should be also much better tuned for the specific workload. Something not really feasible with the allocator.

Quoted 6 lines

Quoted 2 lines

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

That would deserve to be taken to a dumpster and set on fire. The flag combination could be checked in the allocator but the allocator path fast paths are bad enough already.

Quoted 4 lines

If a new allocation/gfp mode is absolutely necessary then I believe that the most reasoanble way forward would be
#define GFP_NO_LOCK ((__force gfp_t)0)

Agree. Even though i see that some code should be adjusted for it. There are a few users of the __get_free_page(0); So, need to double check it:

<snip>

[    0.650351] BUG: kernel NULL pointer dereference, address: 0000000000000010
[    0.651083] #PF: supervisor read access in kernel mode
[    0.651639] #PF: error_code(0x0000) - not-present page
[    0.652200] PGD 0 P4D 0
[    0.652523] Oops: 0000 [#1] SMP NOPTI
[    0.652668] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc7-next-20200930+ #140
[    0.652668] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[    0.652668] RIP: 0010:__find_event_file+0x21/0x80

<snip>

Apart of that. There is a post_alloc_hook(), that gets called from the prep_new_page(). If "debug page alloc enabled", it maps a page for debug purposes invoking kernel_map_pages(). __kernel_map_pages() is ARCH specific. For example, powerpc variant uses sleep-able locks what can be easily converted to raw variant.

Quoted 46 lines · Uladzislau Rezki

Quoted 45 lines · Michal Hocko

Quoted 28 lines · Mel Gorman

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Uladzislau Rezki

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

Quoted 14 lines

include/linux/gfp.h struct vm_area_struct; · line 42

Quoted 10 lines

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +1 -0

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

Quoted 3 lines

That is definitely true. I'm not happy with the GFP flag at all, the comment is at best a damage limiting move. It still would be better for a memory pool to be reserved and sized for critical allocations.

Completely agreed. The only existing usecase is so special cased that a dedicated pool is not only easier to maintain but it should be also much better tuned for the specific workload. Something not really feasible with the allocator.

Quoted 6 lines

Quoted 2 lines

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

That would deserve to be taken to a dumpster and set on fire. The flag combination could be checked in the allocator but the allocator path fast paths are bad enough already.

Quoted 6 lines

Quoted 4 lines

If a new allocation/gfp mode is absolutely necessary then I believe that the most reasoanble way forward would be
#define GFP_NO_LOCK ((__force gfp_t)0)

Agree. Even though i see that some code should be adjusted for it. There are a few users of the __get_free_page(0); So, need to double check it:

Yes, I believe I have pointed that out in the previous discussion.

Quoted 5 lines

Apart of that. There is a post_alloc_hook(), that gets called from the prep_new_page(). If "debug page alloc enabled", it maps a page for debug purposes invoking kernel_map_pages(). __kernel_map_pages() is ARCH specific. For example, powerpc variant uses sleep-able locks what can be easily converted to raw variant.

Yes, there are likely more surprises like that. I am not sure about kasan, page owner (which depens on the stack unwinder) and others which hook into this path.

Quoted 54 lines · Michal Hocko

Quoted 52 lines · Uladzislau Rezki

Quoted 41 lines · Michal Hocko

Quoted 28 lines · Mel Gorman

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Uladzislau Rezki

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

Quoted 14 lines

include/linux/gfp.h struct vm_area_struct; · line 42

Quoted 10 lines

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +1 -0

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

Quoted 3 lines

That is definitely true. I'm not happy with the GFP flag at all, the comment is at best a damage limiting move. It still would be better for a memory pool to be reserved and sized for critical allocations.

Completely agreed. The only existing usecase is so special cased that a dedicated pool is not only easier to maintain but it should be also much better tuned for the specific workload. Something not really feasible with the allocator.

Quoted 3 lines

Quoted 2 lines

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

Quoted 8 lines

Quoted 3 lines

That would deserve to be taken to a dumpster and set on fire. The flag combination could be checked in the allocator but the allocator path fast paths are bad enough already.

If a new allocation/gfp mode is absolutely necessary then I believe that the most reasoanble way forward would be
#define GFP_NO_LOCK ((__force gfp_t)0)

Agree. Even though i see that some code should be adjusted for it. There are a few users of the __get_free_page(0); So, need to double check it:

Quoted 2 lines

Yes, I believe I have pointed that out in the previous discussion.

OK. I spent more time on it. A passed gfp_mask can be adjusted on the entry, that adjustment depends on the gfp_allowed_mask. It can be changed in run-time.

For example during early boot it excludes: __GFP_RECLAIM|__GFP_IO|__GFP_FS flags, what is GFP_KERNEL. So, GFP_KERNEL is adjusted on entry and becomes 0 during early boot phase.

How to distinguish it:

<snip>

(quoted diff) +6 -0

+       /*
+        * gfp_mask can become zero because gfp_allowed_mask changes in run-time.
+        */
+       if (!gfp_mask)
+               alloc_flags |= ALLOC_NO_LOCKS;
+
        gfp_mask &= gfp_allowed_mask;
        alloc_mask = gfp_mask;
        if (!prepare_alloc_pages(gfp_mask, order, preferred_nid, nodemask, &ac, &alloc_mask, &alloc_flags))
<snip>

Quoted 7 lines

Quoted 6 lines

Apart of that. There is a post_alloc_hook(), that gets called from the prep_new_page(). If "debug page alloc enabled", it maps a page for debug purposes invoking kernel_map_pages(). __kernel_map_pages() is ARCH specific. For example, powerpc variant uses sleep-able locks what can be easily converted to raw variant.

Quoted 4 lines

Yes, there are likely more surprises like that. I am not sure about kasan, page owner (which depens on the stack unwinder) and others which hook into this path.

I have checked kasan_alloc_pages(), kernel_poison_pages() both are OK, at least i did not find any locking there. As for set_page_owner(), it requires more attention, since it uses arch specific unwind logic. Though, i spent some time on it and so far have not noticed anything.

Quoted 55 lines · Uladzislau Rezki

Quoted 54 lines · Michal Hocko

Quoted 52 lines · Uladzislau Rezki

Quoted 41 lines · Michal Hocko

Quoted 28 lines · Mel Gorman

Quoted 12 lines · Michal Hocko

Quoted 11 lines · Uladzislau Rezki

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

Quoted 14 lines

include/linux/gfp.h struct vm_area_struct; · line 42

Quoted 10 lines

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +1 -0

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

Quoted 3 lines

That is definitely true. I'm not happy with the GFP flag at all, the comment is at best a damage limiting move. It still would be better for a memory pool to be reserved and sized for critical allocations.

Completely agreed. The only existing usecase is so special cased that a dedicated pool is not only easier to maintain but it should be also much better tuned for the specific workload. Something not really feasible with the allocator.

Quoted 3 lines

Quoted 2 lines

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

Quoted 8 lines

Quoted 3 lines

That would deserve to be taken to a dumpster and set on fire. The flag combination could be checked in the allocator but the allocator path fast paths are bad enough already.

If a new allocation/gfp mode is absolutely necessary then I believe that the most reasoanble way forward would be
#define GFP_NO_LOCK ((__force gfp_t)0)

Agree. Even though i see that some code should be adjusted for it. There are a few users of the __get_free_page(0); So, need to double check it:

Quoted 8 lines

Quoted 2 lines

Yes, I believe I have pointed that out in the previous discussion.

OK. I spent more time on it. A passed gfp_mask can be adjusted on the entry, that adjustment depends on the gfp_allowed_mask. It can be changed in run-time.

For example during early boot it excludes: __GFP_RECLAIM|__GFP_IO|__GFP_FS flags, what is GFP_KERNEL. So, GFP_KERNEL is adjusted on entry and becomes 0 during early boot phase.

Honestly I am not sure how much is GFP_BOOT_MASK still needed. The remaining user of gfp_allowed_mask mask should be only hibernation and I believe this should be removed in long term. Not as trivial because scope API cannot be used for that as it needs a global flag but this is a gross hack that should be implemented differently. It is waiting on my todo list but never got around to that.

Quoted 20 lines

How to distinguish it:

<snip>

(quoted diff) +6 -0

+       /*
+        * gfp_mask can become zero because gfp_allowed_mask changes in run-time.
+        */
+       if (!gfp_mask)
+               alloc_flags |= ALLOC_NO_LOCKS;
+
        gfp_mask &= gfp_allowed_mask;
        alloc_mask = gfp_mask;
        if (!prepare_alloc_pages(gfp_mask, order, preferred_nid, nodemask, &ac, &alloc_mask, &alloc_flags))
<snip>

Quoted 7 lines

Quoted 6 lines

Apart of that. There is a post_alloc_hook(), that gets called from the prep_new_page(). If "debug page alloc enabled", it maps a page for debug purposes invoking kernel_map_pages(). __kernel_map_pages() is ARCH specific. For example, powerpc variant uses sleep-able locks what can be easily converted to raw variant.

Quoted 8 lines

Quoted 4 lines

Yes, there are likely more surprises like that. I am not sure about kasan, page owner (which depens on the stack unwinder) and others which hook into this path.

I have checked kasan_alloc_pages(), kernel_poison_pages() both are OK, at least i did not find any locking there. As for set_page_owner(), it requires more attention, since it uses arch specific unwind logic. Though, i spent some time on it and so far have not noticed anything.

stack depod depends on a lock IIRC. Anyway, this is just showing how this is going to grow in complexity and make future additions harder. A niche usecase is inducing an additional complexity for future maintenance.

Quoted 5 lines · Michal Hocko

Quoted 1 line

+#define ___GFP_NO_LOCKS 0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

That is definitely true. I'm not happy with the GFP flag at all, the comment is at best a damage limiting move. It still would be better for a memory pool to be reserved and sized for critical allocations.

Quoted 2 lines

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

That would deserve to be taken to a dumpster and set on fire. The flag combination could be checked in the allocator but the allocator path fast paths are bad enough already.

Quoted 8 lines · Mel Gorman

Quoted 5 lines · Michal Hocko

Quoted 1 line

+#define ___GFP_NO_LOCKS 0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

Quoted 3 lines

That is definitely true. I'm not happy with the GFP flag at all, the comment is at best a damage limiting move. It still would be better for a memory pool to be reserved and sized for critical allocations.

This is one of the reasons I did a separate allocation function. No GFP flag to leak into general usage.

Quoted 6 lines

Quoted 2 lines

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

That would deserve to be taken to a dumpster and set on fire. The flag combination could be checked in the allocator but the allocator path fast paths are bad enough already.

Isn't that what we have CONFIG_DEBUG_VM for?

Quoted 13 lines · Peter Zijlstra

Quoted 11 lines · Mel Gorman

Quoted 5 lines · Michal Hocko

Quoted 1 line

+#define ___GFP_NO_LOCKS 0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

That is definitely true. I'm not happy with the GFP flag at all, the comment is at best a damage limiting move. It still would be better for a memory pool to be reserved and sized for critical allocations.

Quoted 3 lines

This is one of the reasons I did a separate allocation function. No GFP flag to leak into general usage.

Even a specific function with a hint that "this is for RCU only" will not prevent abuse.

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

That would deserve to be taken to a dumpster and set on fire. The flag combination could be checked in the allocator but the allocator path fast paths are bad enough already.

Quoted 1 line

Isn't that what we have CONFIG_DEBUG_VM for?

It's enabled by default by enough distros that adding too many checks is potentially painful. Granted it would be missed by most benchmarking which tend to control allocations from userspace but a lot of performance problems I see are the "death by a thousand cuts" variety.

Quoted 4 lines

It's enabled by default by enough distros that adding too many checks is potentially painful. Granted it would be missed by most benchmarking which tend to control allocations from userspace but a lot of performance problems I see are the "death by a thousand cuts" variety.

Oh quite agreed, aka death by accounting. But if people are enabling DEBUG options in production kernels, there's something wrong, no?

Should we now go add CONFIG_REALLY_DEBUG_STAY_AWAY_ALREADY options?

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines

It's enabled by default by enough distros that adding too many checks is potentially painful. Granted it would be missed by most benchmarking which tend to control allocations from userspace but a lot of performance problems I see are the "death by a thousand cuts" variety.

Oh quite agreed, aka death by accounting. But if people are enabling DEBUG options in production kernels, there's something wrong, no?

You'd think but historically I believe DEBUG_VM was enabled for some distributions because it made certain classes of problems easier to debug early. There is also a recent trend for enabling various DEBUG options for "hardening" even when they protect very specific corner cases or are for intended for kernel development. I've pushed back where I have an opinion that matters but it's generally corrosive.

Quoted 1 line

Should we now go add CONFIG_REALLY_DEBUG_STAY_AWAY_ALREADY options?

It's heading in that direction :(

Quoted 19 lines · Mel Gorman

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines

It's enabled by default by enough distros that adding too many checks is potentially painful. Granted it would be missed by most benchmarking which tend to control allocations from userspace but a lot of performance problems I see are the "death by a thousand cuts" variety.

Oh quite agreed, aka death by accounting. But if people are enabling DEBUG options in production kernels, there's something wrong, no?

You'd think but historically I believe DEBUG_VM was enabled for some distributions because it made certain classes of problems easier to debug early. There is also a recent trend for enabling various DEBUG options for "hardening" even when they protect very specific corner cases or are for intended for kernel development. I've pushed back where I have an opinion that matters but it's generally corrosive.

Quoted 1 line

Should we now go add CONFIG_REALLY_DEBUG_STAY_AWAY_ALREADY options?

Quoted 1 line

It's heading in that direction :(

Given that you guys have just reiterated yet again that you are very unhappy with either a GFP_ flag or a special function like the one that Peter Zijlstra put together, it would be very helpful if you were to at least voice some level of support for Thomas Gleixner's patchset, which, if accepted, will allow me to solve at least 50% of the problem.

Quoted 20 lines · Paul E. McKenney

Quoted 19 lines · Mel Gorman

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines

It's enabled by default by enough distros that adding too many checks is potentially painful. Granted it would be missed by most benchmarking which tend to control allocations from userspace but a lot of performance problems I see are the "death by a thousand cuts" variety.

Oh quite agreed, aka death by accounting. But if people are enabling DEBUG options in production kernels, there's something wrong, no?

You'd think but historically I believe DEBUG_VM was enabled for some distributions because it made certain classes of problems easier to debug early. There is also a recent trend for enabling various DEBUG options for "hardening" even when they protect very specific corner cases or are for intended for kernel development. I've pushed back where I have an opinion that matters but it's generally corrosive.

Quoted 1 line

Should we now go add CONFIG_REALLY_DEBUG_STAY_AWAY_ALREADY options?

Quoted 7 lines

Quoted 1 line

It's heading in that direction :(

Given that you guys have just reiterated yet again that you are very unhappy with either a GFP_ flag or a special function like the one that Peter Zijlstra put together, it would be very helpful if you were to at least voice some level of support for Thomas Gleixner's patchset, which, if accepted, will allow me to solve at least 50% of the problem.

I read through the series and didn't find anything problematic that had not been covered already. Minimally, avoiding surprises about what preemptible() means in different contexts is nice. While I have not run it through a test grid to check, I'd be very surprised if this was problematic from a performance perspective on a preempt-disabled kernels. Last I checked, the difference between PREEMPT_NONE and PREEMPT_VOLUNTARY was less than 2% *at worst* and I don't think that was due to the preempt accounting.

Quoted 11 lines · Uladzislau Rezki

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

include/linux/gfp.h struct vm_area_struct; · line 42

Quoted 10 lines

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +1 -0

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u

Even if a new gfp flag gains a sufficient traction and support I am _strongly_ opposed against consuming another flag for that. Bit space is limited.

Besides that we certainly do not want to allow craziness like __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we?

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +3 -1

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u
 #ifdef CONFIG_LOCKDEP
-#define ___GFP_NOLOCKDEP       0x800000u
+#define ___GFP_NOLOCKDEP       0x1000000u
 #else
 #define ___GFP_NOLOCKDEP       0
 #endif

@@ -215,16 +216,22 @@ struct vm_area_struct;
  * %__GFP_COMP address compound page metadata.
  *
  * %__GFP_ZERO returns a zeroed page on success.
+ *

Quoted 32 lines · Uladzislau Rezki

Quoted 8 lines

No, I meant going back to idea of new gfp flag, but adjust the implementation in the allocator (different from what you posted in previous version) so that it only looks at the flag after it tries to allocate from pcplist and finds out it's empty. So, no inventing of new page allocator entry points or checks such as the one you wrote above, but adding the new gfp flag in a way that it doesn't affect existing fast paths.

OK. Now i see. Please have a look below at the patch, so we fully understand each other. If that is something that is close to your view or not:

<snip>
t a/include/linux/gfp.h b/include/linux/gfp.h
index c603237e006c..7e613560a502 100644

include/linux/gfp.h +3 -1

@@ -39,8 +39,9 @@ struct vm_area_struct;
 #define ___GFP_HARDWALL                0x100000u
 #define ___GFP_THISNODE                0x200000u
 #define ___GFP_ACCOUNT         0x400000u
+#define ___GFP_NO_LOCKS                0x800000u
 #ifdef CONFIG_LOCKDEP
-#define ___GFP_NOLOCKDEP       0x800000u
+#define ___GFP_NOLOCKDEP       0x1000000u
 #else
 #define ___GFP_NOLOCKDEP       0
 #endif

@@ -215,16 +216,22 @@ struct vm_area_struct;
  * %__GFP_COMP address compound page metadata.
  *
  * %__GFP_ZERO returns a zeroed page on success.
+ *

include/linux/gfp.h struct vm_area_struct; · line 229

Quoted 10 lines

include/linux/gfp.h +5 -0

+ * %__GFP_NO_LOCKS order-0 allocation without sleepable-locks.
+ * It obtains a page from the per-cpu-list and considered as
+ * lock-less. No other actions are performed, thus it returns
+ * NULL if per-cpu-list is empty.
  */
 #define __GFP_NOWARN   ((__force gfp_t)___GFP_NOWARN)
 #define __GFP_COMP     ((__force gfp_t)___GFP_COMP)
 #define __GFP_ZERO     ((__force gfp_t)___GFP_ZERO)
+#define __GFP_NO_LOCKS ((__force gfp_t)___GFP_NO_LOCKS)
 

I'm not a fan of the GFP flag approach simply because we've had cases before where GFP flags were used in inappropriate contexts like __GFP_MEMALLOC which led to a surprising amount of bugs, particularly from out-of-tree drivers but also in-tree drivers. Of course, there are limited GFP flags available too but at least the comment should be as robust as possible. Maybe something like

* %__GFP_NO_LOCKS attempts order-0 allocation without sleepable-locks. It
* attempts to obtain a page without acquiring any spinlocks. This
* should only be used in a context where the holder holds a
* raw_spin_lock that cannot be released for the allocation request.
* This may be necessary in PREEMPT_RT kernels where a
* raw_spin_lock is held which does not sleep tries to acquire a
* spin_lock that can sleep with PREEMPT_RT. This should not be
* confused with GFP_ATOMIC contexts. Like atomic allocation
* requests, there is no guarantee a page will be returned and
* the caller must be able to deal with allocation failures.
* The risk of allocation failure is higher than using GFP_ATOMIC.

It's verbose but it would be hard to misinterpret. I think we're going to go through a period of time before people get familiar with PREEMPT_RT-related hazards as various comments that were true are going to be misleading for a while.

Recently the separate worker thread has been introduced to maintain the local page cache from the regular kernel context, instead of kvfree_rcu() contexts. That was done because a caller of the k[v]free_rcu() can be any context type what is a problem from the allocation point of view.

Quoted 1 line

From the other hand, the lock-less way of obtaining a page has

been introduced and directly injected to the k[v]free_rcu() path.

Therefore it is not important anymore to use a high priority "wq" for the external job that used to fill a page cache ASAP when it was empty.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>

Quoted 6 lines · Uladzislau Rezki (Sony)

Recently the separate worker thread has been introduced to maintain the local page cache from the regular kernel context, instead of kvfree_rcu() contexts. That was done because a caller of the k[v]free_rcu() can be any context type what is a problem from the allocation point of view.

Quoted 8 lines

Quoted 1 line

From the other hand, the lock-less way of obtaining a page has

been introduced and directly injected to the k[v]free_rcu() path.

Therefore it is not important anymore to use a high priority "wq" for the external job that used to fill a page cache ASAP when it was empty.

Signed-off-by Uladzislau Rezki (Sony) <[email protected]>

And I needed to apply the patch below to make this one pass rcutorture scenarios SRCU-P and TREE05. Repeat by:

tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 3 --configs "SRCU-P TREE05" --trust-make

Without the patch below, the system hangs very early in boot.

Please let me know if some other fix would be better.

Thanx, Paul

------------------------------------------------------------------------

kernel/rcu/tree.c +0 -0

index 8ce1ea4..2424e2a 100644

Hello, folk!

This is another iteration of fixing kvfree_rcu() issues related to CONFIG_PROVE_RAW_LOCK_NESTING and CONFIG_PREEMPT_RT configs.

The first discussion is here https://lkml.org/lkml/2020/8/9/195.

- As an outcome of it, there was a proposal from Peter, instead of using a speciall "lock-less" flag it is better to move lock-less access to the pcplist to the separate function.

- To add a special worker thread that does prefetching of pages if a per-cpu page cache is depleted(what is absolutely normal).

As usual, thank you for paying attention to it and your help!

Uladzislau Rezki (Sony) (4):
rcu/tree: Add a work to allocate pages from regular context
mm: Add __rcu_alloc_page_lockless() func.
rcu/tree: use __rcu_alloc_page_lockless() func. rcu/tree: Use schedule_delayed_work() instead of WQ_HIGHPRI queue

 include/linux/gfp.h |  1 +
 kernel/rcu/tree.c   | 90 ++++++++++++++++++++++++---------------------
 mm/page_alloc.c     | 82 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+), 41 deletions(-)

Signature · 1 line

2.20.1

Hello, folk!

This is another iteration of fixing kvfree_rcu() issues related to CONFIG_PROVE_RAW_LOCK_NESTING and CONFIG_PREEMPT_RT configs.

The first discussion is here https://lkml.org/lkml/2020/8/9/195.

- As an outcome of it, there was a proposal from Peter, instead of using a speciall "lock-less" flag it is better to move lock-less access to the pcplist to the separate function.

- To add a special worker thread that does prefetching of pages if a per-cpu page cache is depleted(what is absolutely normal).

As usual, thank you for paying attention to it and your help!

Uladzislau Rezki (Sony) (4):
rcu/tree: Add a work to allocate pages from regular context
mm: Add __rcu_alloc_page_lockless() func.
rcu/tree: use __rcu_alloc_page_lockless() func. rcu/tree: Use schedule_delayed_work() instead of WQ_HIGHPRI queue

 include/linux/gfp.h |  1 +
 kernel/rcu/tree.c   | 90 ++++++++++++++++++++++++---------------------
 mm/page_alloc.c     | 82 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+), 41 deletions(-)

Signature · 1 line

2.20.1