| Message ID | 20241212180208.274813-1-urezki@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Move kvfree_rcu() into SLAB (v2) | expand |
On Thu, 12 Dec 2024, Uladzislau Rezki (Sony) wrote: > This is v2. It is based on the Linux 6.13-rc2. The first version is > here: I do not see any use of internal slab interfaces by this code. It seems to be using rcu internals though. So it would best be placed with the rcu code.
On Thu, Dec 12, 2024 at 10:30:28AM -0800, Christoph Lameter (Ampere) wrote: > On Thu, 12 Dec 2024, Uladzislau Rezki (Sony) wrote: > > > This is v2. It is based on the Linux 6.13-rc2. The first version is > > here: > > I do not see any use of internal slab interfaces by this code. It seems to > be using rcu internals though. So it would best be placed with the rcu > code. > I think, later on there will be integration. This is a step forward to place it under mm where it should be. -- Uladzislau Rezki
On Thu, Dec 12, 2024 at 10:30:28AM -0800, Christoph Lameter (Ampere) wrote: > On Thu, 12 Dec 2024, Uladzislau Rezki (Sony) wrote: > > > This is v2. It is based on the Linux 6.13-rc2. The first version is > > here: > > I do not see any use of internal slab interfaces by this code. It seems to > be using rcu internals though. So it would best be placed with the rcu > code. That is indeed the current state. The point of moving it is to later take advantage of internal slab state. Thanx, Paul
On Thu, Dec 12, 2024 at 11:10:47AM -0800, Paul E. McKenney wrote: > On Thu, Dec 12, 2024 at 10:30:28AM -0800, Christoph Lameter (Ampere) wrote: > > On Thu, 12 Dec 2024, Uladzislau Rezki (Sony) wrote: > > > > > This is v2. It is based on the Linux 6.13-rc2. The first version is > > > here: > > > > I do not see any use of internal slab interfaces by this code. It seems to > > be using rcu internals though. So it would best be placed with the rcu > > code. > > That is indeed the current state. The point of moving it is to later > take advantage of internal slab state. > And, in fact we already have some integrations. For example a barrier has been added for slab caches. -- Uladzislau Rezki
On 12/12/24 19:02, Uladzislau Rezki (Sony) wrote: > Hello! > > This is v2. It is based on the Linux 6.13-rc2. The first version is > here: > > https://lore.kernel.org/linux-mm/20241210164035.3391747-4-urezki@gmail.com/T/ > > The difference between v1 and v2 is that, the preparation process is > done in original place instead and after that there is one final move. Looks good, will include in slab/for-next I think patch 5 should add more explanation to the commit message - the subthread started by Christoph could provide content :) Can you summarize so I can amend the commit log? Also how about a followup patch moving the rcu-tiny implementation of kvfree_call_rcu()? We might also consider moving the kfree_rcu*() entry points from rcupdate.h to slab.h, what do you think, is it a more logical place for them? There's some risk that files that include rcupdate.h and not slab.h would break, so that will need some build testing... Thanks, Vlastimil > Uladzislau Rezki (Sony) (5): > rcu/kvfree: Initialize kvfree_rcu() separately > rcu/kvfree: Move some functions under CONFIG_TINY_RCU > rcu/kvfree: Adjust names passed into trace functions > rcu/kvfree: Adjust a shrinker name > mm/slab: Move kvfree_rcu() into SLAB > > include/linux/slab.h | 1 + > init/main.c | 1 + > kernel/rcu/tree.c | 876 ------------------------------------------ > mm/slab_common.c | 880 +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 882 insertions(+), 876 deletions(-) >
On Sun, Dec 15, 2024 at 06:30:02PM +0100, Vlastimil Babka wrote: > On 12/12/24 19:02, Uladzislau Rezki (Sony) wrote: > > Hello! > > > > This is v2. It is based on the Linux 6.13-rc2. The first version is > > here: > > > > https://lore.kernel.org/linux-mm/20241210164035.3391747-4-urezki@gmail.com/T/ > > > > The difference between v1 and v2 is that, the preparation process is > > done in original place instead and after that there is one final move. > > Looks good, will include in slab/for-next > > I think patch 5 should add more explanation to the commit message - the > subthread started by Christoph could provide content :) Can you summarize so > I can amend the commit log? > > Also how about a followup patch moving the rcu-tiny implementation of > kvfree_call_rcu()? > > We might also consider moving the kfree_rcu*() entry points from rcupdate.h > to slab.h, what do you think, is it a more logical place for them? There's > some risk that files that include rcupdate.h and not slab.h would break, so > that will need some build testing... Moving the RCU Tiny implemention (or maybe even just retiring it in favor of the RCU Tree implementation) and moving the entry points make sense to me! Thanx, Paul > Thanks, > Vlastimil > > > Uladzislau Rezki (Sony) (5): > > rcu/kvfree: Initialize kvfree_rcu() separately > > rcu/kvfree: Move some functions under CONFIG_TINY_RCU > > rcu/kvfree: Adjust names passed into trace functions > > rcu/kvfree: Adjust a shrinker name > > mm/slab: Move kvfree_rcu() into SLAB > > > > include/linux/slab.h | 1 + > > init/main.c | 1 + > > kernel/rcu/tree.c | 876 ------------------------------------------ > > mm/slab_common.c | 880 +++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 882 insertions(+), 876 deletions(-) > > >
On Sun, Dec 15, 2024 at 06:30:02PM +0100, Vlastimil Babka wrote: > On 12/12/24 19:02, Uladzislau Rezki (Sony) wrote: > > Hello! > > > > This is v2. It is based on the Linux 6.13-rc2. The first version is > > here: > > > > https://lore.kernel.org/linux-mm/20241210164035.3391747-4-urezki@gmail.com/T/ > > > > The difference between v1 and v2 is that, the preparation process is > > done in original place instead and after that there is one final move. > > Looks good, will include in slab/for-next > > I think patch 5 should add more explanation to the commit message - the > subthread started by Christoph could provide content :) Can you summarize so > I can amend the commit log? > I will :) > Also how about a followup patch moving the rcu-tiny implementation of > kvfree_call_rcu()? > As, Paul already noted, it would make sense. Or just remove a tiny implementation. > > We might also consider moving the kfree_rcu*() entry points from rcupdate.h > to slab.h, what do you think, is it a more logical place for them? There's > some risk that files that include rcupdate.h and not slab.h would break, so > that will need some build testing... > I agree. I have not moved them in this series, because it requires more testing due to a build break. I can work on this further, so it is not an issue. Thank you for taking this! -- Uladzislau Rezki
On Sun, Dec 15, 2024 at 06:30:02PM +0100, Vlastimil Babka wrote: > On 12/12/24 19:02, Uladzislau Rezki (Sony) wrote: > > Hello! > > > > This is v2. It is based on the Linux 6.13-rc2. The first version is > > here: > > > > https://lore.kernel.org/linux-mm/20241210164035.3391747-4-urezki@gmail.com/T/ > > > > The difference between v1 and v2 is that, the preparation process is > > done in original place instead and after that there is one final move. > > Looks good, will include in slab/for-next > > I think patch 5 should add more explanation to the commit message - the > subthread started by Christoph could provide content :) Can you summarize so > I can amend the commit log? > <snip> mm/slab: Move kvfree_rcu() into SLAB Move kvfree_rcu() functionality to the slab_common.c file. The reason of being kvfree_rcu() functionality as part of SLAB is that, there is a clear trend and need of closer integration. One of the recent example is creating a barrier function for SLAB caches. Another reason is to prevent of having several implementations of RCU machinery for reclaiming objects after a GP. As future steps, it can be more integrated(easier) with SLAB internals. <snip> -- Uladzislau Rezki
On 12/16/24 12:03, Uladzislau Rezki wrote: > On Sun, Dec 15, 2024 at 06:30:02PM +0100, Vlastimil Babka wrote: >> On 12/12/24 19:02, Uladzislau Rezki (Sony) wrote: >> > Hello! >> > >> > This is v2. It is based on the Linux 6.13-rc2. The first version is >> > here: >> > >> > https://lore.kernel.org/linux-mm/20241210164035.3391747-4-urezki@gmail.com/T/ >> > >> > The difference between v1 and v2 is that, the preparation process is >> > done in original place instead and after that there is one final move. >> >> Looks good, will include in slab/for-next >> >> I think patch 5 should add more explanation to the commit message - the >> subthread started by Christoph could provide content :) Can you summarize so >> I can amend the commit log? >> > I will :) > >> Also how about a followup patch moving the rcu-tiny implementation of >> kvfree_call_rcu()? >> > As, Paul already noted, it would make sense. Or just remove a tiny > implementation. AFAICS tiny rcu is for !SMP systems. Do they benefit from the "full" implementation with all the batching etc or would that be unnecessary overhead? >> >> We might also consider moving the kfree_rcu*() entry points from rcupdate.h >> to slab.h, what do you think, is it a more logical place for them? There's >> some risk that files that include rcupdate.h and not slab.h would break, so >> that will need some build testing... >> > I agree. I have not moved them in this series, because it requires more > testing due to a build break. I can work on this further, so it is not > an issue. > > Thank you for taking this! > > -- > Uladzislau Rezki
On Mon, Dec 16, 2024 at 03:20:44PM +0100, Vlastimil Babka wrote: > On 12/16/24 12:03, Uladzislau Rezki wrote: > > On Sun, Dec 15, 2024 at 06:30:02PM +0100, Vlastimil Babka wrote: > >> On 12/12/24 19:02, Uladzislau Rezki (Sony) wrote: > >> > Hello! > >> > > >> > This is v2. It is based on the Linux 6.13-rc2. The first version is > >> > here: > >> > > >> > https://lore.kernel.org/linux-mm/20241210164035.3391747-4-urezki@gmail.com/T/ > >> > > >> > The difference between v1 and v2 is that, the preparation process is > >> > done in original place instead and after that there is one final move. > >> > >> Looks good, will include in slab/for-next > >> > >> I think patch 5 should add more explanation to the commit message - the > >> subthread started by Christoph could provide content :) Can you summarize so > >> I can amend the commit log? > >> > > I will :) > > > >> Also how about a followup patch moving the rcu-tiny implementation of > >> kvfree_call_rcu()? > >> > > As, Paul already noted, it would make sense. Or just remove a tiny > > implementation. > > AFAICS tiny rcu is for !SMP systems. Do they benefit from the "full" > implementation with all the batching etc or would that be unnecessary overhead? > Yes, it is for a really small systems with low amount of memory. I see only one overhead it is about driving objects in pages. For a small system it can be critical because we allocate. From the other hand, for a tiny variant we can modify the normal variant by bypassing batching logic, thus do not consume memory(for Tiny case) i.e. merge it to a normal kvfree_rcu() path. After that we do not depend on CONFIG_RCU_TINY option. Probably we need also to perform some adaptation of regular kvfree_rcu() for a single CPU system. -- Uladzislau Rezki
On 12/16/24 16:41, Uladzislau Rezki wrote: > On Mon, Dec 16, 2024 at 03:20:44PM +0100, Vlastimil Babka wrote: >> On 12/16/24 12:03, Uladzislau Rezki wrote: >> > On Sun, Dec 15, 2024 at 06:30:02PM +0100, Vlastimil Babka wrote: >> >> On 12/12/24 19:02, Uladzislau Rezki (Sony) wrote: >> >> > Hello! >> >> > >> >> > This is v2. It is based on the Linux 6.13-rc2. The first version is >> >> > here: >> >> > >> >> > https://lore.kernel.org/linux-mm/20241210164035.3391747-4-urezki@gmail.com/T/ >> >> > >> >> > The difference between v1 and v2 is that, the preparation process is >> >> > done in original place instead and after that there is one final move. >> >> >> >> Looks good, will include in slab/for-next >> >> >> >> I think patch 5 should add more explanation to the commit message - the >> >> subthread started by Christoph could provide content :) Can you summarize so >> >> I can amend the commit log? >> >> >> > I will :) >> > >> >> Also how about a followup patch moving the rcu-tiny implementation of >> >> kvfree_call_rcu()? >> >> >> > As, Paul already noted, it would make sense. Or just remove a tiny >> > implementation. >> >> AFAICS tiny rcu is for !SMP systems. Do they benefit from the "full" >> implementation with all the batching etc or would that be unnecessary overhead? >> > Yes, it is for a really small systems with low amount of memory. I see > only one overhead it is about driving objects in pages. For a small > system it can be critical because we allocate. > > From the other hand, for a tiny variant we can modify the normal variant > by bypassing batching logic, thus do not consume memory(for Tiny case) > i.e. merge it to a normal kvfree_rcu() path. Maybe we could change it to use CONFIG_SLUB_TINY as that has similar use case (less memory usage on low memory system, tradeoff for worse performance). > After that we do not depend on CONFIG_RCU_TINY option. Probably we need > also to perform some adaptation of regular kvfree_rcu() for a single CPU > system. > > -- > Uladzislau Rezki
On Mon, Dec 16, 2024 at 04:44:41PM +0100, Vlastimil Babka wrote: > On 12/16/24 16:41, Uladzislau Rezki wrote: > > On Mon, Dec 16, 2024 at 03:20:44PM +0100, Vlastimil Babka wrote: > >> On 12/16/24 12:03, Uladzislau Rezki wrote: > >> > On Sun, Dec 15, 2024 at 06:30:02PM +0100, Vlastimil Babka wrote: > >> >> On 12/12/24 19:02, Uladzislau Rezki (Sony) wrote: > >> >> > Hello! > >> >> > > >> >> > This is v2. It is based on the Linux 6.13-rc2. The first version is > >> >> > here: > >> >> > > >> >> > https://lore.kernel.org/linux-mm/20241210164035.3391747-4-urezki@gmail.com/T/ > >> >> > > >> >> > The difference between v1 and v2 is that, the preparation process is > >> >> > done in original place instead and after that there is one final move. > >> >> > >> >> Looks good, will include in slab/for-next > >> >> > >> >> I think patch 5 should add more explanation to the commit message - the > >> >> subthread started by Christoph could provide content :) Can you summarize so > >> >> I can amend the commit log? > >> >> > >> > I will :) > >> > > >> >> Also how about a followup patch moving the rcu-tiny implementation of > >> >> kvfree_call_rcu()? > >> >> > >> > As, Paul already noted, it would make sense. Or just remove a tiny > >> > implementation. > >> > >> AFAICS tiny rcu is for !SMP systems. Do they benefit from the "full" > >> implementation with all the batching etc or would that be unnecessary overhead? > >> > > Yes, it is for a really small systems with low amount of memory. I see > > only one overhead it is about driving objects in pages. For a small > > system it can be critical because we allocate. > > > > From the other hand, for a tiny variant we can modify the normal variant > > by bypassing batching logic, thus do not consume memory(for Tiny case) > > i.e. merge it to a normal kvfree_rcu() path. > > Maybe we could change it to use CONFIG_SLUB_TINY as that has similar use > case (less memory usage on low memory system, tradeoff for worse performance). > Yep, i also was thinking about that without saying it :) -- Uladzislau Rezki
On Mon, Dec 16, 2024 at 04:55:06PM +0100, Uladzislau Rezki wrote: > On Mon, Dec 16, 2024 at 04:44:41PM +0100, Vlastimil Babka wrote: > > On 12/16/24 16:41, Uladzislau Rezki wrote: > > > On Mon, Dec 16, 2024 at 03:20:44PM +0100, Vlastimil Babka wrote: > > >> On 12/16/24 12:03, Uladzislau Rezki wrote: > > >> > On Sun, Dec 15, 2024 at 06:30:02PM +0100, Vlastimil Babka wrote: > > >> >> On 12/12/24 19:02, Uladzislau Rezki (Sony) wrote: > > >> >> > Hello! > > >> >> > > > >> >> > This is v2. It is based on the Linux 6.13-rc2. The first version is > > >> >> > here: > > >> >> > > > >> >> > https://lore.kernel.org/linux-mm/20241210164035.3391747-4-urezki@gmail.com/T/ > > >> >> > > > >> >> > The difference between v1 and v2 is that, the preparation process is > > >> >> > done in original place instead and after that there is one final move. > > >> >> > > >> >> Looks good, will include in slab/for-next > > >> >> > > >> >> I think patch 5 should add more explanation to the commit message - the > > >> >> subthread started by Christoph could provide content :) Can you summarize so > > >> >> I can amend the commit log? > > >> >> > > >> > I will :) > > >> > > > >> >> Also how about a followup patch moving the rcu-tiny implementation of > > >> >> kvfree_call_rcu()? > > >> >> > > >> > As, Paul already noted, it would make sense. Or just remove a tiny > > >> > implementation. > > >> > > >> AFAICS tiny rcu is for !SMP systems. Do they benefit from the "full" > > >> implementation with all the batching etc or would that be unnecessary overhead? > > >> > > > Yes, it is for a really small systems with low amount of memory. I see > > > only one overhead it is about driving objects in pages. For a small > > > system it can be critical because we allocate. > > > > > > From the other hand, for a tiny variant we can modify the normal variant > > > by bypassing batching logic, thus do not consume memory(for Tiny case) > > > i.e. merge it to a normal kvfree_rcu() path. > > > > Maybe we could change it to use CONFIG_SLUB_TINY as that has similar use > > case (less memory usage on low memory system, tradeoff for worse performance). > > > Yep, i also was thinking about that without saying it :) Works for me as well! Thanx, Paul