mbox series

[RFC,v1,0/5] Move kvfree_rcu() into SLAB

Message ID 20241210164035.3391747-1-urezki@gmail.com (mailing list archive)
Headers show
Series Move kvfree_rcu() into SLAB | expand

Message

Uladzislau Rezki Dec. 10, 2024, 4:40 p.m. UTC
Hello!

This series is based on v6.12 kernel. It is an attempt to move the kvfree_rcu()
into MM from the kernel/rcu/ place. I split the series into a few patches so it
is easier to follow a migration process.

As a result of this series, the main functionality is located under MM.

Uladzislau Rezki (Sony) (5):
  rcu/kvfree: Temporary reclaim over call_rcu()
  mm/slab: Copy main data structures of kvfree_rcu()
  mm/slab: Copy internal functions of kvfree_rcu()
  mm/slab: Copy a function of kvfree_rcu() initialization
  mm/slab: Move kvfree_rcu() into SLAB

 include/linux/slab.h |   1 +
 init/main.c          |   1 +
 kernel/rcu/tree.c    | 866 ------------------------------------------
 mm/slab_common.c     | 875 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 877 insertions(+), 866 deletions(-)

Comments

Paul E. McKenney Dec. 11, 2024, 4:12 p.m. UTC | #1
On Tue, Dec 10, 2024 at 05:40:30PM +0100, Uladzislau Rezki (Sony) wrote:
> Hello!
> 
> This series is based on v6.12 kernel. It is an attempt to move the kvfree_rcu()
> into MM from the kernel/rcu/ place. I split the series into a few patches so it
> is easier to follow a migration process.
> 
> As a result of this series, the main functionality is located under MM.
> 
> Uladzislau Rezki (Sony) (5):

Tested-by: Paul E. McKenney <paulmck@kernel.org>

>   rcu/kvfree: Temporary reclaim over call_rcu()
>   mm/slab: Copy main data structures of kvfree_rcu()
>   mm/slab: Copy internal functions of kvfree_rcu()
>   mm/slab: Copy a function of kvfree_rcu() initialization
>   mm/slab: Move kvfree_rcu() into SLAB
> 
>  include/linux/slab.h |   1 +
>  init/main.c          |   1 +
>  kernel/rcu/tree.c    | 866 ------------------------------------------
>  mm/slab_common.c     | 875 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 877 insertions(+), 866 deletions(-)
> 
> -- 
> 2.39.5
> 
> 
>
Vlastimil Babka Dec. 12, 2024, 10:30 a.m. UTC | #2
On 12/10/24 17:40, Uladzislau Rezki (Sony) wrote:
> Hello!

Hi and thanks!

> This series is based on v6.12 kernel.

Could it be rebased to v6.13-rc1, which is a basis for most -next branches?
Right now patch 5 doesn't apply on v6.13-rc1.

Please also Cc all slab maintainers/reviewers.

> It is an attempt to move the kvfree_rcu()
> into MM from the kernel/rcu/ place. I split the series into a few patches so it
> is easier to follow a migration process.

I think this is not the best approach. The individual diffs are not easy to
follow because they copy code or delete code separately, and not move it in
a single commit. I get a much better overview when I diff the whole series
against baseline, then git highlights pure moves and local changes nicely.

Having moves recorded properly would also make it possible for "git blame
-C" to show changes that were made in the old file before the move, but with
copy and deletion in separate commits it doesn't work.
(but note it seems it doesn't work so great even if I squash everything to
one patch - were the functions reodered?)

And with this approach you also need the temporary changes.

What I think could work better is to do:
- preparatory changes in the existing location
  - splitting out kvfree_rcu_init() and calling separately in start_kernel()
  - renaming shrinkers
  - adjusting the names passed to trace_rcu_...()
  - maybe even adding the CONFIG_TINY_RCU guards even if redundant
- one big move of code between files, hopefully needing no or minimal
adjustments after the preparatory steps

Makes sense?

Thanks,
Vlastimil

> As a result of this series, the main functionality is located under MM.
> 
> Uladzislau Rezki (Sony) (5):
>   rcu/kvfree: Temporary reclaim over call_rcu()
>   mm/slab: Copy main data structures of kvfree_rcu()
>   mm/slab: Copy internal functions of kvfree_rcu()
>   mm/slab: Copy a function of kvfree_rcu() initialization
>   mm/slab: Move kvfree_rcu() into SLAB
> 
>  include/linux/slab.h |   1 +
>  init/main.c          |   1 +
>  kernel/rcu/tree.c    | 866 ------------------------------------------
>  mm/slab_common.c     | 875 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 877 insertions(+), 866 deletions(-)
>
Uladzislau Rezki Dec. 12, 2024, 6:04 p.m. UTC | #3
On Thu, Dec 12, 2024 at 11:30:36AM +0100, Vlastimil Babka wrote:
> On 12/10/24 17:40, Uladzislau Rezki (Sony) wrote:
> > Hello!
> 
> Hi and thanks!
> 
> > This series is based on v6.12 kernel.
> 
> Could it be rebased to v6.13-rc1, which is a basis for most -next branches?
> Right now patch 5 doesn't apply on v6.13-rc1.
> 
> Please also Cc all slab maintainers/reviewers.
> 
> > It is an attempt to move the kvfree_rcu()
> > into MM from the kernel/rcu/ place. I split the series into a few patches so it
> > is easier to follow a migration process.
> 
> I think this is not the best approach. The individual diffs are not easy to
> follow because they copy code or delete code separately, and not move it in
> a single commit. I get a much better overview when I diff the whole series
> against baseline, then git highlights pure moves and local changes nicely.
> 
> Having moves recorded properly would also make it possible for "git blame
> -C" to show changes that were made in the old file before the move, but with
> copy and deletion in separate commits it doesn't work.
> (but note it seems it doesn't work so great even if I squash everything to
> one patch - were the functions reodered?)
> 
> And with this approach you also need the temporary changes.
> 
> What I think could work better is to do:
> - preparatory changes in the existing location
>   - splitting out kvfree_rcu_init() and calling separately in start_kernel()
>   - renaming shrinkers
>   - adjusting the names passed to trace_rcu_...()
>   - maybe even adding the CONFIG_TINY_RCU guards even if redundant
> - one big move of code between files, hopefully needing no or minimal
> adjustments after the preparatory steps
> 
> Makes sense?
> 
See v2. We can go that way, so it makes sense to me.

Thank you.

--
Uladzislau Rezki