Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 09.08.2018 00:31, Dave Chinner wrote: > On Wed, Aug 08, 2018 at 12:27:34PM +0200, Michal Hocko wrote: >> [CC Josh - the whole series is >> http://lkml.kernel.org/r/153365347929.19074.12509495712735843805.stgit@localhost.localdomain] >> >> On Wed 08-08-18 13:17:44, Kirill Tkhai wrote: >>> On 08.08.2018 10:20, Michal Hocko wrote: On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. The last time somebody tried to do this there was a pushback due to kernel tinyfication. So this should really give some numbers about the code size increase. Also why can't we make this depend on MMU. Is anybody else than the reclaim asking for unconditional SRCU usage? >>> >>> I don't know one. The size numbers (sparc64) are: >>> >>> $ size image.srcu.disabled >>>textdata bss dec hex filename >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled >>> $ size image.srcu.enabled >>>textdata bss dec hex filename >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled >>> The difference is: 15158625-15116156 = 42469 ~41Kb >>> >>> Please, see the measurement details to my answer to Stephen. >>> Btw. I totaly agree with Steven. This is a very poor changelog. It is trivial to see what the patch does but it is far from clear why it is doing that and why we cannot go other ways. >>> We possibly can go another way, and there is comment to [2/10] about this. >>> Percpu rwsem may be used instead, the only thing, it is worse, is it will >>> make shrink_slab() wait unregistering shrinkers, while srcu-based >>> implementation does not require this. >> >> Well, if unregisterring doesn't do anything subtle - e.g. an allocation >> or take locks which depend on allocation - and we can guarantee that >> then blocking shrink_slab shouldn't be a big deal. > > unregister_shrinker() already blocks shrink_slab - taking a rwsem in > write mode blocks all readers - so using a per-cpu rwsem doesn't > introduce anything new or unexpected. I'd like to see numbers of the > different methods before anything else. The difference is percpu_rw_semaphore makes readers to wait till RCU grace period is finished. Sometimes this takes unpredictable time on big machines with many CPUs, which is not good. > IMO, the big deal is that the split unregister mechanism seems to > imply superblock shrinkers can be called during sb teardown or > /after/ the filesystem has been torn down in memory (i.e. after > ->put_super() is called). That's a change of behaviour, but it's > left to the filesystem to detect and handle that condition. That's > exceedingly subtle and looks like a recipe for disaster to me. I > note that XFS hasn't been updated to detect and avoid this landmine. > > And, FWIW, filesystems with multiple shrinkers (e.g. XFS as 3 per > mount) will take the SCRU penalty multiple times during unmount, and > potentially be exposed to multiple different "use during/after > teardown" race conditions. > >> It is subtle though. >> Maybe subtle enough to make unconditional SRCU worth it. This all should >> be in the changelog though. > > IMO, we've had enough recent bugs to deal with from shrinkers being > called before the filesystem is set up and from trying to handle > allocation errors during setup. Do we really want to make shrinker > shutdown just as prone to mismanagement and subtle, hard to hit > bugs? I don't think we do - unmount is simply not a critical > performance path. There are possible different situations, people use linux like they want. Imagine, you want to reboot NFS server, but you want to enter clients and umount them over ssh, and the time is critical. Something like this. I believe there are many examples, people need this.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 09.08.2018 00:31, Dave Chinner wrote: > On Wed, Aug 08, 2018 at 12:27:34PM +0200, Michal Hocko wrote: >> [CC Josh - the whole series is >> http://lkml.kernel.org/r/153365347929.19074.12509495712735843805.stgit@localhost.localdomain] >> >> On Wed 08-08-18 13:17:44, Kirill Tkhai wrote: >>> On 08.08.2018 10:20, Michal Hocko wrote: On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. The last time somebody tried to do this there was a pushback due to kernel tinyfication. So this should really give some numbers about the code size increase. Also why can't we make this depend on MMU. Is anybody else than the reclaim asking for unconditional SRCU usage? >>> >>> I don't know one. The size numbers (sparc64) are: >>> >>> $ size image.srcu.disabled >>>textdata bss dec hex filename >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled >>> $ size image.srcu.enabled >>>textdata bss dec hex filename >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled >>> The difference is: 15158625-15116156 = 42469 ~41Kb >>> >>> Please, see the measurement details to my answer to Stephen. >>> Btw. I totaly agree with Steven. This is a very poor changelog. It is trivial to see what the patch does but it is far from clear why it is doing that and why we cannot go other ways. >>> We possibly can go another way, and there is comment to [2/10] about this. >>> Percpu rwsem may be used instead, the only thing, it is worse, is it will >>> make shrink_slab() wait unregistering shrinkers, while srcu-based >>> implementation does not require this. >> >> Well, if unregisterring doesn't do anything subtle - e.g. an allocation >> or take locks which depend on allocation - and we can guarantee that >> then blocking shrink_slab shouldn't be a big deal. > > unregister_shrinker() already blocks shrink_slab - taking a rwsem in > write mode blocks all readers - so using a per-cpu rwsem doesn't > introduce anything new or unexpected. I'd like to see numbers of the > different methods before anything else. The difference is percpu_rw_semaphore makes readers to wait till RCU grace period is finished. Sometimes this takes unpredictable time on big machines with many CPUs, which is not good. > IMO, the big deal is that the split unregister mechanism seems to > imply superblock shrinkers can be called during sb teardown or > /after/ the filesystem has been torn down in memory (i.e. after > ->put_super() is called). That's a change of behaviour, but it's > left to the filesystem to detect and handle that condition. That's > exceedingly subtle and looks like a recipe for disaster to me. I > note that XFS hasn't been updated to detect and avoid this landmine. > > And, FWIW, filesystems with multiple shrinkers (e.g. XFS as 3 per > mount) will take the SCRU penalty multiple times during unmount, and > potentially be exposed to multiple different "use during/after > teardown" race conditions. > >> It is subtle though. >> Maybe subtle enough to make unconditional SRCU worth it. This all should >> be in the changelog though. > > IMO, we've had enough recent bugs to deal with from shrinkers being > called before the filesystem is set up and from trying to handle > allocation errors during setup. Do we really want to make shrinker > shutdown just as prone to mismanagement and subtle, hard to hit > bugs? I don't think we do - unmount is simply not a critical > performance path. There are possible different situations, people use linux like they want. Imagine, you want to reboot NFS server, but you want to enter clients and umount them over ssh, and the time is critical. Something like this. I believe there are many examples, people need this.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 05:07:08PM -0700, Matthew Wilcox wrote: > On Thu, Aug 09, 2018 at 07:31:25AM +1000, Dave Chinner wrote: > > IMO, we've had enough recent bugs to deal with from shrinkers being > > called before the filesystem is set up and from trying to handle > > allocation errors during setup. Do we really want to make shrinker > > shutdown just as prone to mismanagement and subtle, hard to hit > > bugs? I don't think we do - unmount is simply not a critical > > performance path. > > It's never been performance critical for me, but I'm not so sure that > there aren't container workloads which unmount filesystems multiple > times per second. What? Why would they do that? Who cares about tear-down speeds? Start up speeds I can kind of understand...
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 05:07:08PM -0700, Matthew Wilcox wrote: > On Thu, Aug 09, 2018 at 07:31:25AM +1000, Dave Chinner wrote: > > IMO, we've had enough recent bugs to deal with from shrinkers being > > called before the filesystem is set up and from trying to handle > > allocation errors during setup. Do we really want to make shrinker > > shutdown just as prone to mismanagement and subtle, hard to hit > > bugs? I don't think we do - unmount is simply not a critical > > performance path. > > It's never been performance critical for me, but I'm not so sure that > there aren't container workloads which unmount filesystems multiple > times per second. What? Why would they do that? Who cares about tear-down speeds? Start up speeds I can kind of understand...
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Thu, Aug 09, 2018 at 07:31:25AM +1000, Dave Chinner wrote: > IMO, we've had enough recent bugs to deal with from shrinkers being > called before the filesystem is set up and from trying to handle > allocation errors during setup. Do we really want to make shrinker > shutdown just as prone to mismanagement and subtle, hard to hit > bugs? I don't think we do - unmount is simply not a critical > performance path. It's never been performance critical for me, but I'm not so sure that there aren't container workloads which unmount filesystems multiple times per second.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Thu, Aug 09, 2018 at 07:31:25AM +1000, Dave Chinner wrote: > IMO, we've had enough recent bugs to deal with from shrinkers being > called before the filesystem is set up and from trying to handle > allocation errors during setup. Do we really want to make shrinker > shutdown just as prone to mismanagement and subtle, hard to hit > bugs? I don't think we do - unmount is simply not a critical > performance path. It's never been performance critical for me, but I'm not so sure that there aren't container workloads which unmount filesystems multiple times per second.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 04:02:29PM -0700, Shakeel Butt wrote: > On Wed, Aug 8, 2018 at 11:02 AM Josh Triplett wrote: > > > > On Wed, Aug 08, 2018 at 07:30:13PM +0300, Kirill Tkhai wrote: > > > On 08.08.2018 19:23, Kirill Tkhai wrote: > > > > On 08.08.2018 19:13, Josh Triplett wrote: > > > >> On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: > > > >>> On 08.08.2018 10:20, Michal Hocko wrote: > > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > > > > This patch kills all CONFIG_SRCU defines and > > > > the code under !CONFIG_SRCU. > > > > > > The last time somebody tried to do this there was a pushback due to > > > kernel tinyfication. So this should really give some numbers about > > > the > > > code size increase. Also why can't we make this depend on MMU. Is > > > anybody else than the reclaim asking for unconditional SRCU usage? > > > >>> > > > >>> I don't know one. The size numbers (sparc64) are: > > > >>> > > > >>> $ size image.srcu.disabled > > > >>>text data bss dec hex filename > > > >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > > > >>> $ size image.srcu.enabled > > > >>>text data bss dec hex filename > > > >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > > > >>> The difference is: 15158625-15116156 = 42469 ~41Kb > > > >> > > > >> 41k is a *substantial* size increase. However, can you compare > > > >> tinyconfig with and without this patch? That may have a smaller change. > > > > > > > > $ size image.srcu.disabled > > > >textdata bss dec hex filename > > > > 1105900 195456 63232 1364588 14d26c image.srcu.disabled > > > > > > > > $ size image.srcu.enabled > > > >textdata bss dec hex filename > > > > 1106960 195528 63232 1365720 14d6d8 image.srcu.enabled > > > > > > > > 1365720-1364588 = 1132 ~ 1Kb > > > > > > 1Kb is not huge size. It looks as not a big price for writing generic code > > > for only case (now some places have CONFIG_SRCU and !CONFIG_SRCU variants, > > > e.g. drivers/base/core.c). What do you think? > > > > That's a little more reasonable than 41k, likely because of > > CONFIG_TINY_SRCU. That's still not ideal, though. And as far as I can > > tell, the *only* two pieces of core code that use SRCU are > > drivers/base/core.c and kernel/notifier.c, and the latter is exclusively > > code to use notifiers with SRCU, not notifiers wanting to use SRCU > > themselves. So, as far as I can tell, this would really just save a > > couple of small #ifdef sections in drivers/base/core.c, and I think > > those #ifdef sections could be simplified even further. That doesn't > > seem worth it at all. > > Hi Josh, the motivation behind enabling SRCU is not to simplify the > code in drivers/base/core.c but rather not to introduce similar ifdefs > in mm/vmscan.c for shrinker traversals. Leaving aside the comment someone made about sticking with rwsem for this, I honestly hope that someday the shrinker is optional too. :)
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 04:02:29PM -0700, Shakeel Butt wrote: > On Wed, Aug 8, 2018 at 11:02 AM Josh Triplett wrote: > > > > On Wed, Aug 08, 2018 at 07:30:13PM +0300, Kirill Tkhai wrote: > > > On 08.08.2018 19:23, Kirill Tkhai wrote: > > > > On 08.08.2018 19:13, Josh Triplett wrote: > > > >> On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: > > > >>> On 08.08.2018 10:20, Michal Hocko wrote: > > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > > > > This patch kills all CONFIG_SRCU defines and > > > > the code under !CONFIG_SRCU. > > > > > > The last time somebody tried to do this there was a pushback due to > > > kernel tinyfication. So this should really give some numbers about > > > the > > > code size increase. Also why can't we make this depend on MMU. Is > > > anybody else than the reclaim asking for unconditional SRCU usage? > > > >>> > > > >>> I don't know one. The size numbers (sparc64) are: > > > >>> > > > >>> $ size image.srcu.disabled > > > >>>text data bss dec hex filename > > > >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > > > >>> $ size image.srcu.enabled > > > >>>text data bss dec hex filename > > > >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > > > >>> The difference is: 15158625-15116156 = 42469 ~41Kb > > > >> > > > >> 41k is a *substantial* size increase. However, can you compare > > > >> tinyconfig with and without this patch? That may have a smaller change. > > > > > > > > $ size image.srcu.disabled > > > >textdata bss dec hex filename > > > > 1105900 195456 63232 1364588 14d26c image.srcu.disabled > > > > > > > > $ size image.srcu.enabled > > > >textdata bss dec hex filename > > > > 1106960 195528 63232 1365720 14d6d8 image.srcu.enabled > > > > > > > > 1365720-1364588 = 1132 ~ 1Kb > > > > > > 1Kb is not huge size. It looks as not a big price for writing generic code > > > for only case (now some places have CONFIG_SRCU and !CONFIG_SRCU variants, > > > e.g. drivers/base/core.c). What do you think? > > > > That's a little more reasonable than 41k, likely because of > > CONFIG_TINY_SRCU. That's still not ideal, though. And as far as I can > > tell, the *only* two pieces of core code that use SRCU are > > drivers/base/core.c and kernel/notifier.c, and the latter is exclusively > > code to use notifiers with SRCU, not notifiers wanting to use SRCU > > themselves. So, as far as I can tell, this would really just save a > > couple of small #ifdef sections in drivers/base/core.c, and I think > > those #ifdef sections could be simplified even further. That doesn't > > seem worth it at all. > > Hi Josh, the motivation behind enabling SRCU is not to simplify the > code in drivers/base/core.c but rather not to introduce similar ifdefs > in mm/vmscan.c for shrinker traversals. Leaving aside the comment someone made about sticking with rwsem for this, I honestly hope that someday the shrinker is optional too. :)
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 8, 2018 at 11:02 AM Josh Triplett wrote: > > On Wed, Aug 08, 2018 at 07:30:13PM +0300, Kirill Tkhai wrote: > > On 08.08.2018 19:23, Kirill Tkhai wrote: > > > On 08.08.2018 19:13, Josh Triplett wrote: > > >> On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: > > >>> On 08.08.2018 10:20, Michal Hocko wrote: > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > > > This patch kills all CONFIG_SRCU defines and > > > the code under !CONFIG_SRCU. > > > > The last time somebody tried to do this there was a pushback due to > > kernel tinyfication. So this should really give some numbers about the > > code size increase. Also why can't we make this depend on MMU. Is > > anybody else than the reclaim asking for unconditional SRCU usage? > > >>> > > >>> I don't know one. The size numbers (sparc64) are: > > >>> > > >>> $ size image.srcu.disabled > > >>>text data bss dec hex filename > > >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > > >>> $ size image.srcu.enabled > > >>>text data bss dec hex filename > > >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > > >>> The difference is: 15158625-15116156 = 42469 ~41Kb > > >> > > >> 41k is a *substantial* size increase. However, can you compare > > >> tinyconfig with and without this patch? That may have a smaller change. > > > > > > $ size image.srcu.disabled > > >textdata bss dec hex filename > > > 1105900 195456 63232 1364588 14d26c image.srcu.disabled > > > > > > $ size image.srcu.enabled > > >textdata bss dec hex filename > > > 1106960 195528 63232 1365720 14d6d8 image.srcu.enabled > > > > > > 1365720-1364588 = 1132 ~ 1Kb > > > > 1Kb is not huge size. It looks as not a big price for writing generic code > > for only case (now some places have CONFIG_SRCU and !CONFIG_SRCU variants, > > e.g. drivers/base/core.c). What do you think? > > That's a little more reasonable than 41k, likely because of > CONFIG_TINY_SRCU. That's still not ideal, though. And as far as I can > tell, the *only* two pieces of core code that use SRCU are > drivers/base/core.c and kernel/notifier.c, and the latter is exclusively > code to use notifiers with SRCU, not notifiers wanting to use SRCU > themselves. So, as far as I can tell, this would really just save a > couple of small #ifdef sections in drivers/base/core.c, and I think > those #ifdef sections could be simplified even further. That doesn't > seem worth it at all. Hi Josh, the motivation behind enabling SRCU is not to simplify the code in drivers/base/core.c but rather not to introduce similar ifdefs in mm/vmscan.c for shrinker traversals. Shakeel
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 8, 2018 at 11:02 AM Josh Triplett wrote: > > On Wed, Aug 08, 2018 at 07:30:13PM +0300, Kirill Tkhai wrote: > > On 08.08.2018 19:23, Kirill Tkhai wrote: > > > On 08.08.2018 19:13, Josh Triplett wrote: > > >> On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: > > >>> On 08.08.2018 10:20, Michal Hocko wrote: > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > > > This patch kills all CONFIG_SRCU defines and > > > the code under !CONFIG_SRCU. > > > > The last time somebody tried to do this there was a pushback due to > > kernel tinyfication. So this should really give some numbers about the > > code size increase. Also why can't we make this depend on MMU. Is > > anybody else than the reclaim asking for unconditional SRCU usage? > > >>> > > >>> I don't know one. The size numbers (sparc64) are: > > >>> > > >>> $ size image.srcu.disabled > > >>>text data bss dec hex filename > > >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > > >>> $ size image.srcu.enabled > > >>>text data bss dec hex filename > > >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > > >>> The difference is: 15158625-15116156 = 42469 ~41Kb > > >> > > >> 41k is a *substantial* size increase. However, can you compare > > >> tinyconfig with and without this patch? That may have a smaller change. > > > > > > $ size image.srcu.disabled > > >textdata bss dec hex filename > > > 1105900 195456 63232 1364588 14d26c image.srcu.disabled > > > > > > $ size image.srcu.enabled > > >textdata bss dec hex filename > > > 1106960 195528 63232 1365720 14d6d8 image.srcu.enabled > > > > > > 1365720-1364588 = 1132 ~ 1Kb > > > > 1Kb is not huge size. It looks as not a big price for writing generic code > > for only case (now some places have CONFIG_SRCU and !CONFIG_SRCU variants, > > e.g. drivers/base/core.c). What do you think? > > That's a little more reasonable than 41k, likely because of > CONFIG_TINY_SRCU. That's still not ideal, though. And as far as I can > tell, the *only* two pieces of core code that use SRCU are > drivers/base/core.c and kernel/notifier.c, and the latter is exclusively > code to use notifiers with SRCU, not notifiers wanting to use SRCU > themselves. So, as far as I can tell, this would really just save a > couple of small #ifdef sections in drivers/base/core.c, and I think > those #ifdef sections could be simplified even further. That doesn't > seem worth it at all. Hi Josh, the motivation behind enabling SRCU is not to simplify the code in drivers/base/core.c but rather not to introduce similar ifdefs in mm/vmscan.c for shrinker traversals. Shakeel
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 12:27:34PM +0200, Michal Hocko wrote: > [CC Josh - the whole series is > http://lkml.kernel.org/r/153365347929.19074.12509495712735843805.stgit@localhost.localdomain] > > On Wed 08-08-18 13:17:44, Kirill Tkhai wrote: > > On 08.08.2018 10:20, Michal Hocko wrote: > > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > > >> This patch kills all CONFIG_SRCU defines and > > >> the code under !CONFIG_SRCU. > > > > > > The last time somebody tried to do this there was a pushback due to > > > kernel tinyfication. So this should really give some numbers about the > > > code size increase. Also why can't we make this depend on MMU. Is > > > anybody else than the reclaim asking for unconditional SRCU usage? > > > > I don't know one. The size numbers (sparc64) are: > > > > $ size image.srcu.disabled > >textdata bss dec hex filename > > 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > > $ size image.srcu.enabled > >textdata bss dec hex filename > > 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > > The difference is: 15158625-15116156 = 42469 ~41Kb > > > > Please, see the measurement details to my answer to Stephen. > > > > > Btw. I totaly agree with Steven. This is a very poor changelog. It is > > > trivial to see what the patch does but it is far from clear why it is > > > doing that and why we cannot go other ways. > > We possibly can go another way, and there is comment to [2/10] about this. > > Percpu rwsem may be used instead, the only thing, it is worse, is it will > > make shrink_slab() wait unregistering shrinkers, while srcu-based > > implementation does not require this. > > Well, if unregisterring doesn't do anything subtle - e.g. an allocation > or take locks which depend on allocation - and we can guarantee that > then blocking shrink_slab shouldn't be a big deal. unregister_shrinker() already blocks shrink_slab - taking a rwsem in write mode blocks all readers - so using a per-cpu rwsem doesn't introduce anything new or unexpected. I'd like to see numbers of the different methods before anything else. IMO, the big deal is that the split unregister mechanism seems to imply superblock shrinkers can be called during sb teardown or /after/ the filesystem has been torn down in memory (i.e. after ->put_super() is called). That's a change of behaviour, but it's left to the filesystem to detect and handle that condition. That's exceedingly subtle and looks like a recipe for disaster to me. I note that XFS hasn't been updated to detect and avoid this landmine. And, FWIW, filesystems with multiple shrinkers (e.g. XFS as 3 per mount) will take the SCRU penalty multiple times during unmount, and potentially be exposed to multiple different "use during/after teardown" race conditions. > It is subtle though. > Maybe subtle enough to make unconditional SRCU worth it. This all should > be in the changelog though. IMO, we've had enough recent bugs to deal with from shrinkers being called before the filesystem is set up and from trying to handle allocation errors during setup. Do we really want to make shrinker shutdown just as prone to mismanagement and subtle, hard to hit bugs? I don't think we do - unmount is simply not a critical performance path. Cheers, Dave. -- Dave Chinner da...@fromorbit.com
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 12:27:34PM +0200, Michal Hocko wrote: > [CC Josh - the whole series is > http://lkml.kernel.org/r/153365347929.19074.12509495712735843805.stgit@localhost.localdomain] > > On Wed 08-08-18 13:17:44, Kirill Tkhai wrote: > > On 08.08.2018 10:20, Michal Hocko wrote: > > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > > >> This patch kills all CONFIG_SRCU defines and > > >> the code under !CONFIG_SRCU. > > > > > > The last time somebody tried to do this there was a pushback due to > > > kernel tinyfication. So this should really give some numbers about the > > > code size increase. Also why can't we make this depend on MMU. Is > > > anybody else than the reclaim asking for unconditional SRCU usage? > > > > I don't know one. The size numbers (sparc64) are: > > > > $ size image.srcu.disabled > >textdata bss dec hex filename > > 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > > $ size image.srcu.enabled > >textdata bss dec hex filename > > 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > > The difference is: 15158625-15116156 = 42469 ~41Kb > > > > Please, see the measurement details to my answer to Stephen. > > > > > Btw. I totaly agree with Steven. This is a very poor changelog. It is > > > trivial to see what the patch does but it is far from clear why it is > > > doing that and why we cannot go other ways. > > We possibly can go another way, and there is comment to [2/10] about this. > > Percpu rwsem may be used instead, the only thing, it is worse, is it will > > make shrink_slab() wait unregistering shrinkers, while srcu-based > > implementation does not require this. > > Well, if unregisterring doesn't do anything subtle - e.g. an allocation > or take locks which depend on allocation - and we can guarantee that > then blocking shrink_slab shouldn't be a big deal. unregister_shrinker() already blocks shrink_slab - taking a rwsem in write mode blocks all readers - so using a per-cpu rwsem doesn't introduce anything new or unexpected. I'd like to see numbers of the different methods before anything else. IMO, the big deal is that the split unregister mechanism seems to imply superblock shrinkers can be called during sb teardown or /after/ the filesystem has been torn down in memory (i.e. after ->put_super() is called). That's a change of behaviour, but it's left to the filesystem to detect and handle that condition. That's exceedingly subtle and looks like a recipe for disaster to me. I note that XFS hasn't been updated to detect and avoid this landmine. And, FWIW, filesystems with multiple shrinkers (e.g. XFS as 3 per mount) will take the SCRU penalty multiple times during unmount, and potentially be exposed to multiple different "use during/after teardown" race conditions. > It is subtle though. > Maybe subtle enough to make unconditional SRCU worth it. This all should > be in the changelog though. IMO, we've had enough recent bugs to deal with from shrinkers being called before the filesystem is set up and from trying to handle allocation errors during setup. Do we really want to make shrinker shutdown just as prone to mismanagement and subtle, hard to hit bugs? I don't think we do - unmount is simply not a critical performance path. Cheers, Dave. -- Dave Chinner da...@fromorbit.com
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 07:30:13PM +0300, Kirill Tkhai wrote: > On 08.08.2018 19:23, Kirill Tkhai wrote: > > On 08.08.2018 19:13, Josh Triplett wrote: > >> On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: > >>> On 08.08.2018 10:20, Michal Hocko wrote: > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > > This patch kills all CONFIG_SRCU defines and > > the code under !CONFIG_SRCU. > > The last time somebody tried to do this there was a pushback due to > kernel tinyfication. So this should really give some numbers about the > code size increase. Also why can't we make this depend on MMU. Is > anybody else than the reclaim asking for unconditional SRCU usage? > >>> > >>> I don't know one. The size numbers (sparc64) are: > >>> > >>> $ size image.srcu.disabled > >>>text data bss dec hex filename > >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > >>> $ size image.srcu.enabled > >>>text data bss dec hex filename > >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > >>> The difference is: 15158625-15116156 = 42469 ~41Kb > >> > >> 41k is a *substantial* size increase. However, can you compare > >> tinyconfig with and without this patch? That may have a smaller change. > > > > $ size image.srcu.disabled > >textdata bss dec hex filename > > 1105900 195456 63232 1364588 14d26c image.srcu.disabled > > > > $ size image.srcu.enabled > >textdata bss dec hex filename > > 1106960 195528 63232 1365720 14d6d8 image.srcu.enabled > > > > 1365720-1364588 = 1132 ~ 1Kb > > 1Kb is not huge size. It looks as not a big price for writing generic code > for only case (now some places have CONFIG_SRCU and !CONFIG_SRCU variants, > e.g. drivers/base/core.c). What do you think? That's a little more reasonable than 41k, likely because of CONFIG_TINY_SRCU. That's still not ideal, though. And as far as I can tell, the *only* two pieces of core code that use SRCU are drivers/base/core.c and kernel/notifier.c, and the latter is exclusively code to use notifiers with SRCU, not notifiers wanting to use SRCU themselves. So, as far as I can tell, this would really just save a couple of small #ifdef sections in drivers/base/core.c, and I think those #ifdef sections could be simplified even further. That doesn't seem worth it at all.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 07:30:13PM +0300, Kirill Tkhai wrote: > On 08.08.2018 19:23, Kirill Tkhai wrote: > > On 08.08.2018 19:13, Josh Triplett wrote: > >> On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: > >>> On 08.08.2018 10:20, Michal Hocko wrote: > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > > This patch kills all CONFIG_SRCU defines and > > the code under !CONFIG_SRCU. > > The last time somebody tried to do this there was a pushback due to > kernel tinyfication. So this should really give some numbers about the > code size increase. Also why can't we make this depend on MMU. Is > anybody else than the reclaim asking for unconditional SRCU usage? > >>> > >>> I don't know one. The size numbers (sparc64) are: > >>> > >>> $ size image.srcu.disabled > >>>text data bss dec hex filename > >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > >>> $ size image.srcu.enabled > >>>text data bss dec hex filename > >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > >>> The difference is: 15158625-15116156 = 42469 ~41Kb > >> > >> 41k is a *substantial* size increase. However, can you compare > >> tinyconfig with and without this patch? That may have a smaller change. > > > > $ size image.srcu.disabled > >textdata bss dec hex filename > > 1105900 195456 63232 1364588 14d26c image.srcu.disabled > > > > $ size image.srcu.enabled > >textdata bss dec hex filename > > 1106960 195528 63232 1365720 14d6d8 image.srcu.enabled > > > > 1365720-1364588 = 1132 ~ 1Kb > > 1Kb is not huge size. It looks as not a big price for writing generic code > for only case (now some places have CONFIG_SRCU and !CONFIG_SRCU variants, > e.g. drivers/base/core.c). What do you think? That's a little more reasonable than 41k, likely because of CONFIG_TINY_SRCU. That's still not ideal, though. And as far as I can tell, the *only* two pieces of core code that use SRCU are drivers/base/core.c and kernel/notifier.c, and the latter is exclusively code to use notifiers with SRCU, not notifiers wanting to use SRCU themselves. So, as far as I can tell, this would really just save a couple of small #ifdef sections in drivers/base/core.c, and I think those #ifdef sections could be simplified even further. That doesn't seem worth it at all.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 19:23, Kirill Tkhai wrote: > On 08.08.2018 19:13, Josh Triplett wrote: >> On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: >>> On 08.08.2018 10:20, Michal Hocko wrote: On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. The last time somebody tried to do this there was a pushback due to kernel tinyfication. So this should really give some numbers about the code size increase. Also why can't we make this depend on MMU. Is anybody else than the reclaim asking for unconditional SRCU usage? >>> >>> I don't know one. The size numbers (sparc64) are: >>> >>> $ size image.srcu.disabled >>>textdata bss dec hex filename >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled >>> $ size image.srcu.enabled >>>textdata bss dec hex filename >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled >>> The difference is: 15158625-15116156 = 42469 ~41Kb >> >> 41k is a *substantial* size increase. However, can you compare >> tinyconfig with and without this patch? That may have a smaller change. > > $ size image.srcu.disabled >text data bss dec hex filename > 1105900195456 63232 1364588 14d26c image.srcu.disabled > > $ size image.srcu.enabled >text data bss dec hex filename > 1106960195528 63232 1365720 14d6d8 image.srcu.enabled > > 1365720-1364588 = 1132 ~ 1Kb 1Kb is not huge size. It looks as not a big price for writing generic code for only case (now some places have CONFIG_SRCU and !CONFIG_SRCU variants, e.g. drivers/base/core.c). What do you think?
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 19:23, Kirill Tkhai wrote: > On 08.08.2018 19:13, Josh Triplett wrote: >> On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: >>> On 08.08.2018 10:20, Michal Hocko wrote: On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. The last time somebody tried to do this there was a pushback due to kernel tinyfication. So this should really give some numbers about the code size increase. Also why can't we make this depend on MMU. Is anybody else than the reclaim asking for unconditional SRCU usage? >>> >>> I don't know one. The size numbers (sparc64) are: >>> >>> $ size image.srcu.disabled >>>textdata bss dec hex filename >>> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled >>> $ size image.srcu.enabled >>>textdata bss dec hex filename >>> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled >>> The difference is: 15158625-15116156 = 42469 ~41Kb >> >> 41k is a *substantial* size increase. However, can you compare >> tinyconfig with and without this patch? That may have a smaller change. > > $ size image.srcu.disabled >text data bss dec hex filename > 1105900195456 63232 1364588 14d26c image.srcu.disabled > > $ size image.srcu.enabled >text data bss dec hex filename > 1106960195528 63232 1365720 14d6d8 image.srcu.enabled > > 1365720-1364588 = 1132 ~ 1Kb 1Kb is not huge size. It looks as not a big price for writing generic code for only case (now some places have CONFIG_SRCU and !CONFIG_SRCU variants, e.g. drivers/base/core.c). What do you think?
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 19:13, Josh Triplett wrote: > On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: >> On 08.08.2018 10:20, Michal Hocko wrote: >>> On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: This patch kills all CONFIG_SRCU defines and the code under !CONFIG_SRCU. >>> >>> The last time somebody tried to do this there was a pushback due to >>> kernel tinyfication. So this should really give some numbers about the >>> code size increase. Also why can't we make this depend on MMU. Is >>> anybody else than the reclaim asking for unconditional SRCU usage? >> >> I don't know one. The size numbers (sparc64) are: >> >> $ size image.srcu.disabled >>text data bss dec hex filename >> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled >> $ size image.srcu.enabled >>text data bss dec hex filename >> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled >> The difference is: 15158625-15116156 = 42469 ~41Kb > > 41k is a *substantial* size increase. However, can you compare > tinyconfig with and without this patch? That may have a smaller change. $ size image.srcu.disabled textdata bss dec hex filename 1105900 195456 63232 1364588 14d26c image.srcu.disabled $ size image.srcu.enabled textdata bss dec hex filename 1106960 195528 63232 1365720 14d6d8 image.srcu.enabled 1365720-1364588 = 1132 ~ 1Kb
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 19:13, Josh Triplett wrote: > On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: >> On 08.08.2018 10:20, Michal Hocko wrote: >>> On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: This patch kills all CONFIG_SRCU defines and the code under !CONFIG_SRCU. >>> >>> The last time somebody tried to do this there was a pushback due to >>> kernel tinyfication. So this should really give some numbers about the >>> code size increase. Also why can't we make this depend on MMU. Is >>> anybody else than the reclaim asking for unconditional SRCU usage? >> >> I don't know one. The size numbers (sparc64) are: >> >> $ size image.srcu.disabled >>text data bss dec hex filename >> 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled >> $ size image.srcu.enabled >>text data bss dec hex filename >> 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled >> The difference is: 15158625-15116156 = 42469 ~41Kb > > 41k is a *substantial* size increase. However, can you compare > tinyconfig with and without this patch? That may have a smaller change. $ size image.srcu.disabled textdata bss dec hex filename 1105900 195456 63232 1364588 14d26c image.srcu.disabled $ size image.srcu.enabled textdata bss dec hex filename 1106960 195528 63232 1365720 14d6d8 image.srcu.enabled 1365720-1364588 = 1132 ~ 1Kb
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: > On 08.08.2018 10:20, Michal Hocko wrote: > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > >> This patch kills all CONFIG_SRCU defines and > >> the code under !CONFIG_SRCU. > > > > The last time somebody tried to do this there was a pushback due to > > kernel tinyfication. So this should really give some numbers about the > > code size increase. Also why can't we make this depend on MMU. Is > > anybody else than the reclaim asking for unconditional SRCU usage? > > I don't know one. The size numbers (sparc64) are: > > $ size image.srcu.disabled >text data bss dec hex filename > 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > $ size image.srcu.enabled >text data bss dec hex filename > 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > The difference is: 15158625-15116156 = 42469 ~41Kb 41k is a *substantial* size increase. However, can you compare tinyconfig with and without this patch? That may have a smaller change.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Wed, Aug 08, 2018 at 01:17:44PM +0300, Kirill Tkhai wrote: > On 08.08.2018 10:20, Michal Hocko wrote: > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > >> This patch kills all CONFIG_SRCU defines and > >> the code under !CONFIG_SRCU. > > > > The last time somebody tried to do this there was a pushback due to > > kernel tinyfication. So this should really give some numbers about the > > code size increase. Also why can't we make this depend on MMU. Is > > anybody else than the reclaim asking for unconditional SRCU usage? > > I don't know one. The size numbers (sparc64) are: > > $ size image.srcu.disabled >text data bss dec hex filename > 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > $ size image.srcu.enabled >text data bss dec hex filename > 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > The difference is: 15158625-15116156 = 42469 ~41Kb 41k is a *substantial* size increase. However, can you compare tinyconfig with and without this patch? That may have a smaller change.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
Hi Kirill, On Wed, 8 Aug 2018 12:59:40 +0300 Kirill Tkhai wrote: > > On 08.08.2018 04:08, Stephen Rothwell wrote: > > > > So what sort of overheads (in terms of code size and performance) are > > we adding by having SRCU enabled where it used not to be? > > SRCU is unconditionally enabled for x86, so I had to use another arch > (sparc64) > to check the size difference. The config, I used to compile, is attached, SRCU > was enabled via: > > diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig > index 2d58c26bff9a..6e9116e356d4 100644 > --- a/arch/sparc/Kconfig > +++ b/arch/sparc/Kconfig > @@ -15,6 +15,7 @@ config SPARC > select ARCH_MIGHT_HAVE_PC_PARPORT if SPARC64 && PCI > select ARCH_MIGHT_HAVE_PC_SERIO > select OF > + select SRCU > select OF_PROMTREE > select HAVE_IDE > select HAVE_OPROFILE > > $ size image.srcu.disabled >text data bss dec hex filename > 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > > $ size image.srcu.enabled >text data bss dec hex filename > 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > > The difference is: 15158625-15116156 = 42469 ~41Kb Thanks for that. > I have not ideas about performance overhead measurements. If you have ideas, > where they may occur, please say. At the first sight, there should not be > a problem, since SRCU is enabled in x86 by default. I have no idea, just asking questions that might be relevant for platforms where SRCU is normally disabled. -- Cheers, Stephen Rothwell pgpB1OJYCGZmY.pgp Description: OpenPGP digital signature
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
Hi Kirill, On Wed, 8 Aug 2018 12:59:40 +0300 Kirill Tkhai wrote: > > On 08.08.2018 04:08, Stephen Rothwell wrote: > > > > So what sort of overheads (in terms of code size and performance) are > > we adding by having SRCU enabled where it used not to be? > > SRCU is unconditionally enabled for x86, so I had to use another arch > (sparc64) > to check the size difference. The config, I used to compile, is attached, SRCU > was enabled via: > > diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig > index 2d58c26bff9a..6e9116e356d4 100644 > --- a/arch/sparc/Kconfig > +++ b/arch/sparc/Kconfig > @@ -15,6 +15,7 @@ config SPARC > select ARCH_MIGHT_HAVE_PC_PARPORT if SPARC64 && PCI > select ARCH_MIGHT_HAVE_PC_SERIO > select OF > + select SRCU > select OF_PROMTREE > select HAVE_IDE > select HAVE_OPROFILE > > $ size image.srcu.disabled >text data bss dec hex filename > 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > > $ size image.srcu.enabled >text data bss dec hex filename > 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > > The difference is: 15158625-15116156 = 42469 ~41Kb Thanks for that. > I have not ideas about performance overhead measurements. If you have ideas, > where they may occur, please say. At the first sight, there should not be > a problem, since SRCU is enabled in x86 by default. I have no idea, just asking questions that might be relevant for platforms where SRCU is normally disabled. -- Cheers, Stephen Rothwell pgpB1OJYCGZmY.pgp Description: OpenPGP digital signature
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
[CC Josh - the whole series is http://lkml.kernel.org/r/153365347929.19074.12509495712735843805.stgit@localhost.localdomain] On Wed 08-08-18 13:17:44, Kirill Tkhai wrote: > On 08.08.2018 10:20, Michal Hocko wrote: > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > >> This patch kills all CONFIG_SRCU defines and > >> the code under !CONFIG_SRCU. > > > > The last time somebody tried to do this there was a pushback due to > > kernel tinyfication. So this should really give some numbers about the > > code size increase. Also why can't we make this depend on MMU. Is > > anybody else than the reclaim asking for unconditional SRCU usage? > > I don't know one. The size numbers (sparc64) are: > > $ size image.srcu.disabled >text data bss dec hex filename > 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > $ size image.srcu.enabled >text data bss dec hex filename > 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > The difference is: 15158625-15116156 = 42469 ~41Kb > > Please, see the measurement details to my answer to Stephen. > > > Btw. I totaly agree with Steven. This is a very poor changelog. It is > > trivial to see what the patch does but it is far from clear why it is > > doing that and why we cannot go other ways. > We possibly can go another way, and there is comment to [2/10] about this. > Percpu rwsem may be used instead, the only thing, it is worse, is it will > make shrink_slab() wait unregistering shrinkers, while srcu-based > implementation does not require this. Well, if unregisterring doesn't do anything subtle - e.g. an allocation or take locks which depend on allocation - and we can guarantee that then blocking shrink_slab shouldn't be a big deal. It is subtle though. Maybe subtle enough to make unconditional SRCU worth it. This all should be in the changelog though. > This may be not a big problem. > But, if SRCU is real problem for embedded people, I really don't want they > hate me in the future because of this, so please CC someone if you know :) I guess Josh was trying to pursue kernel tinification. -- Michal Hocko SUSE Labs
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
[CC Josh - the whole series is http://lkml.kernel.org/r/153365347929.19074.12509495712735843805.stgit@localhost.localdomain] On Wed 08-08-18 13:17:44, Kirill Tkhai wrote: > On 08.08.2018 10:20, Michal Hocko wrote: > > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > >> This patch kills all CONFIG_SRCU defines and > >> the code under !CONFIG_SRCU. > > > > The last time somebody tried to do this there was a pushback due to > > kernel tinyfication. So this should really give some numbers about the > > code size increase. Also why can't we make this depend on MMU. Is > > anybody else than the reclaim asking for unconditional SRCU usage? > > I don't know one. The size numbers (sparc64) are: > > $ size image.srcu.disabled >text data bss dec hex filename > 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled > $ size image.srcu.enabled >text data bss dec hex filename > 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled > The difference is: 15158625-15116156 = 42469 ~41Kb > > Please, see the measurement details to my answer to Stephen. > > > Btw. I totaly agree with Steven. This is a very poor changelog. It is > > trivial to see what the patch does but it is far from clear why it is > > doing that and why we cannot go other ways. > We possibly can go another way, and there is comment to [2/10] about this. > Percpu rwsem may be used instead, the only thing, it is worse, is it will > make shrink_slab() wait unregistering shrinkers, while srcu-based > implementation does not require this. Well, if unregisterring doesn't do anything subtle - e.g. an allocation or take locks which depend on allocation - and we can guarantee that then blocking shrink_slab shouldn't be a big deal. It is subtle though. Maybe subtle enough to make unconditional SRCU worth it. This all should be in the changelog though. > This may be not a big problem. > But, if SRCU is real problem for embedded people, I really don't want they > hate me in the future because of this, so please CC someone if you know :) I guess Josh was trying to pursue kernel tinification. -- Michal Hocko SUSE Labs
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 10:20, Michal Hocko wrote: > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: >> This patch kills all CONFIG_SRCU defines and >> the code under !CONFIG_SRCU. > > The last time somebody tried to do this there was a pushback due to > kernel tinyfication. So this should really give some numbers about the > code size increase. Also why can't we make this depend on MMU. Is > anybody else than the reclaim asking for unconditional SRCU usage? I don't know one. The size numbers (sparc64) are: $ size image.srcu.disabled textdata bss dec hex filename 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled $ size image.srcu.enabled textdata bss dec hex filename 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled The difference is: 15158625-15116156 = 42469 ~41Kb Please, see the measurement details to my answer to Stephen. > Btw. I totaly agree with Steven. This is a very poor changelog. It is > trivial to see what the patch does but it is far from clear why it is > doing that and why we cannot go other ways. We possibly can go another way, and there is comment to [2/10] about this. Percpu rwsem may be used instead, the only thing, it is worse, is it will make shrink_slab() wait unregistering shrinkers, while srcu-based implementation does not require this. This may be not a big problem. But, if SRCU is real problem for embedded people, I really don't want they hate me in the future because of this, so please CC someone if you know :) Kirill
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 10:20, Michal Hocko wrote: > On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: >> This patch kills all CONFIG_SRCU defines and >> the code under !CONFIG_SRCU. > > The last time somebody tried to do this there was a pushback due to > kernel tinyfication. So this should really give some numbers about the > code size increase. Also why can't we make this depend on MMU. Is > anybody else than the reclaim asking for unconditional SRCU usage? I don't know one. The size numbers (sparc64) are: $ size image.srcu.disabled textdata bss dec hex filename 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled $ size image.srcu.enabled textdata bss dec hex filename 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled The difference is: 15158625-15116156 = 42469 ~41Kb Please, see the measurement details to my answer to Stephen. > Btw. I totaly agree with Steven. This is a very poor changelog. It is > trivial to see what the patch does but it is far from clear why it is > doing that and why we cannot go other ways. We possibly can go another way, and there is comment to [2/10] about this. Percpu rwsem may be used instead, the only thing, it is worse, is it will make shrink_slab() wait unregistering shrinkers, while srcu-based implementation does not require this. This may be not a big problem. But, if SRCU is real problem for embedded people, I really don't want they hate me in the future because of this, so please CC someone if you know :) Kirill
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 04:08, Stephen Rothwell wrote: > Hi Kirill, > > On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: >> >> This patch kills all CONFIG_SRCU defines and >> the code under !CONFIG_SRCU. >> >> Signed-off-by: Kirill Tkhai > > So what sort of overheads (in terms of code size and performance) are > we adding by having SRCU enabled where it used not to be? SRCU is unconditionally enabled for x86, so I had to use another arch (sparc64) to check the size difference. The config, I used to compile, is attached, SRCU was enabled via: diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig index 2d58c26bff9a..6e9116e356d4 100644 --- a/arch/sparc/Kconfig +++ b/arch/sparc/Kconfig @@ -15,6 +15,7 @@ config SPARC select ARCH_MIGHT_HAVE_PC_PARPORT if SPARC64 && PCI select ARCH_MIGHT_HAVE_PC_SERIO select OF + select SRCU select OF_PROMTREE select HAVE_IDE select HAVE_OPROFILE $ size image.srcu.disabled textdata bss dec hex filename 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled $ size image.srcu.enabled textdata bss dec hex filename 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled The difference is: 15158625-15116156 = 42469 ~41Kb I have not ideas about performance overhead measurements. If you have ideas, where they may occur, please say. At the first sight, there should not be a problem, since SRCU is enabled in x86 by default. # # Automatically generated file; DO NOT EDIT. # Linux/sparc64 4.18.0-rc8 Kernel Configuration # # # Compiler: sparc64-linux-gnu-gcc-6 (Debian 6.4.0-16) 6.4.0 20180406 # CONFIG_CC_IS_GCC=y CONFIG_GCC_VERSION=60400 CONFIG_CLANG_VERSION=0 CONFIG_IRQ_WORK=y # # General setup # CONFIG_INIT_ENV_ARG_LIMIT=32 # CONFIG_COMPILE_TEST is not set CONFIG_LOCALVERSION="" # CONFIG_LOCALVERSION_AUTO is not set CONFIG_BUILD_SALT="" CONFIG_DEFAULT_HOSTNAME="(none)" CONFIG_SYSVIPC=y CONFIG_SYSVIPC_SYSCTL=y CONFIG_POSIX_MQUEUE=y CONFIG_POSIX_MQUEUE_SYSCTL=y CONFIG_CROSS_MEMORY_ATTACH=y CONFIG_USELIB=y # CONFIG_AUDIT is not set CONFIG_HAVE_ARCH_AUDITSYSCALL=y # # IRQ subsystem # CONFIG_GENERIC_IRQ_SHOW=y CONFIG_IRQ_PREFLOW_FASTEOI=y CONFIG_IRQ_DOMAIN=y CONFIG_GENERIC_MSI_IRQ=y CONFIG_SPARSE_IRQ=y # CONFIG_GENERIC_IRQ_DEBUGFS is not set CONFIG_ARCH_CLOCKSOURCE_DATA=y CONFIG_GENERIC_TIME_VSYSCALL=y CONFIG_GENERIC_CLOCKEVENTS=y # # Timers subsystem # CONFIG_TICK_ONESHOT=y CONFIG_NO_HZ_COMMON=y # CONFIG_HZ_PERIODIC is not set CONFIG_NO_HZ_IDLE=y # CONFIG_NO_HZ_FULL is not set CONFIG_NO_HZ=y CONFIG_HIGH_RES_TIMERS=y # CONFIG_PREEMPT_NONE is not set CONFIG_PREEMPT_VOLUNTARY=y # CONFIG_PREEMPT is not set # # CPU/Task time and stats accounting # CONFIG_TICK_CPU_ACCOUNTING=y # CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set # CONFIG_BSD_PROCESS_ACCT is not set # CONFIG_TASKSTATS is not set CONFIG_CPU_ISOLATION=y # # RCU Subsystem # CONFIG_TREE_RCU=y CONFIG_RCU_EXPERT=y CONFIG_SRCU=y CONFIG_TREE_SRCU=y CONFIG_RCU_STALL_COMMON=y CONFIG_RCU_NEED_SEGCBLIST=y CONFIG_RCU_FANOUT=64 CONFIG_RCU_FANOUT_LEAF=16 # CONFIG_RCU_FAST_NO_HZ is not set # CONFIG_RCU_NOCB_CPU is not set # CONFIG_IKCONFIG is not set CONFIG_LOG_BUF_SHIFT=18 CONFIG_LOG_CPU_MAX_BUF_SHIFT=12 CONFIG_PRINTK_SAFE_LOG_BUF_SHIFT=13 # CONFIG_CGROUPS is not set CONFIG_NAMESPACES=y CONFIG_UTS_NS=y CONFIG_IPC_NS=y # CONFIG_USER_NS is not set CONFIG_PID_NS=y CONFIG_NET_NS=y # CONFIG_CHECKPOINT_RESTORE is not set # CONFIG_SCHED_AUTOGROUP is not set # CONFIG_SYSFS_DEPRECATED is not set CONFIG_RELAY=y CONFIG_BLK_DEV_INITRD=y CONFIG_INITRAMFS_SOURCE="" CONFIG_RD_GZIP=y CONFIG_RD_BZIP2=y CONFIG_RD_LZMA=y CONFIG_RD_XZ=y CONFIG_RD_LZO=y CONFIG_RD_LZ4=y CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y # CONFIG_CC_OPTIMIZE_FOR_SIZE is not set CONFIG_SYSCTL=y CONFIG_HAVE_UID16=y CONFIG_SYSCTL_EXCEPTION_TRACE=y CONFIG_BPF=y CONFIG_EXPERT=y CONFIG_UID16=y CONFIG_MULTIUSER=y CONFIG_SGETMASK_SYSCALL=y CONFIG_SYSFS_SYSCALL=y # CONFIG_SYSCTL_SYSCALL is not set CONFIG_FHANDLE=y CONFIG_POSIX_TIMERS=y CONFIG_PRINTK=y CONFIG_PRINTK_NMI=y CONFIG_BUG=y CONFIG_ELF_CORE=y CONFIG_BASE_FULL=y CONFIG_FUTEX=y CONFIG_FUTEX_PI=y CONFIG_EPOLL=y CONFIG_SIGNALFD=y CONFIG_TIMERFD=y CONFIG_EVENTFD=y CONFIG_SHMEM=y CONFIG_AIO=y CONFIG_ADVISE_SYSCALLS=y CONFIG_MEMBARRIER=y CONFIG_KALLSYMS=y # CONFIG_KALLSYMS_ALL is not set CONFIG_KALLSYMS_BASE_RELATIVE=y # CONFIG_BPF_SYSCALL is not set # CONFIG_USERFAULTFD is not set # CONFIG_EMBEDDED is not set CONFIG_HAVE_PERF_EVENTS=y CONFIG_PERF_USE_VMALLOC=y # CONFIG_PC104 is not set # # Kernel Performance Events And Counters # # CONFIG_PERF_EVENTS is not set CONFIG_VM_EVENT_COUNTERS=y # CONFIG_COMPAT_BRK is not set CONFIG_SLAB=y # CONFIG_SLUB is not set # CONFIG_SLOB is not set CONFIG_SLAB_MERGE_DEFAULT=y # CONFIG_SLAB_FREELIST_RANDOM is not set CONFIG_PROFILING=y CONFIG_TRACEPOINTS=y CONFIG_64BIT=y CONFIG_SPARC=y CONFIG_SPARC64=y CONFIG_ARCH_DEFCONFIG="arch/sparc/configs/sparc64_defconfig" CONFIG_ARCH_PROC_KCORE_TEXT=y
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 04:08, Stephen Rothwell wrote: > Hi Kirill, > > On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: >> >> This patch kills all CONFIG_SRCU defines and >> the code under !CONFIG_SRCU. >> >> Signed-off-by: Kirill Tkhai > > So what sort of overheads (in terms of code size and performance) are > we adding by having SRCU enabled where it used not to be? SRCU is unconditionally enabled for x86, so I had to use another arch (sparc64) to check the size difference. The config, I used to compile, is attached, SRCU was enabled via: diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig index 2d58c26bff9a..6e9116e356d4 100644 --- a/arch/sparc/Kconfig +++ b/arch/sparc/Kconfig @@ -15,6 +15,7 @@ config SPARC select ARCH_MIGHT_HAVE_PC_PARPORT if SPARC64 && PCI select ARCH_MIGHT_HAVE_PC_SERIO select OF + select SRCU select OF_PROMTREE select HAVE_IDE select HAVE_OPROFILE $ size image.srcu.disabled textdata bss dec hex filename 5117546 8030506 1968104 15116156 e6a77c image.srcu.disabled $ size image.srcu.enabled textdata bss dec hex filename 5126175 8064346 1968104 15158625 e74d61 image.srcu.enabled The difference is: 15158625-15116156 = 42469 ~41Kb I have not ideas about performance overhead measurements. If you have ideas, where they may occur, please say. At the first sight, there should not be a problem, since SRCU is enabled in x86 by default. # # Automatically generated file; DO NOT EDIT. # Linux/sparc64 4.18.0-rc8 Kernel Configuration # # # Compiler: sparc64-linux-gnu-gcc-6 (Debian 6.4.0-16) 6.4.0 20180406 # CONFIG_CC_IS_GCC=y CONFIG_GCC_VERSION=60400 CONFIG_CLANG_VERSION=0 CONFIG_IRQ_WORK=y # # General setup # CONFIG_INIT_ENV_ARG_LIMIT=32 # CONFIG_COMPILE_TEST is not set CONFIG_LOCALVERSION="" # CONFIG_LOCALVERSION_AUTO is not set CONFIG_BUILD_SALT="" CONFIG_DEFAULT_HOSTNAME="(none)" CONFIG_SYSVIPC=y CONFIG_SYSVIPC_SYSCTL=y CONFIG_POSIX_MQUEUE=y CONFIG_POSIX_MQUEUE_SYSCTL=y CONFIG_CROSS_MEMORY_ATTACH=y CONFIG_USELIB=y # CONFIG_AUDIT is not set CONFIG_HAVE_ARCH_AUDITSYSCALL=y # # IRQ subsystem # CONFIG_GENERIC_IRQ_SHOW=y CONFIG_IRQ_PREFLOW_FASTEOI=y CONFIG_IRQ_DOMAIN=y CONFIG_GENERIC_MSI_IRQ=y CONFIG_SPARSE_IRQ=y # CONFIG_GENERIC_IRQ_DEBUGFS is not set CONFIG_ARCH_CLOCKSOURCE_DATA=y CONFIG_GENERIC_TIME_VSYSCALL=y CONFIG_GENERIC_CLOCKEVENTS=y # # Timers subsystem # CONFIG_TICK_ONESHOT=y CONFIG_NO_HZ_COMMON=y # CONFIG_HZ_PERIODIC is not set CONFIG_NO_HZ_IDLE=y # CONFIG_NO_HZ_FULL is not set CONFIG_NO_HZ=y CONFIG_HIGH_RES_TIMERS=y # CONFIG_PREEMPT_NONE is not set CONFIG_PREEMPT_VOLUNTARY=y # CONFIG_PREEMPT is not set # # CPU/Task time and stats accounting # CONFIG_TICK_CPU_ACCOUNTING=y # CONFIG_VIRT_CPU_ACCOUNTING_GEN is not set # CONFIG_BSD_PROCESS_ACCT is not set # CONFIG_TASKSTATS is not set CONFIG_CPU_ISOLATION=y # # RCU Subsystem # CONFIG_TREE_RCU=y CONFIG_RCU_EXPERT=y CONFIG_SRCU=y CONFIG_TREE_SRCU=y CONFIG_RCU_STALL_COMMON=y CONFIG_RCU_NEED_SEGCBLIST=y CONFIG_RCU_FANOUT=64 CONFIG_RCU_FANOUT_LEAF=16 # CONFIG_RCU_FAST_NO_HZ is not set # CONFIG_RCU_NOCB_CPU is not set # CONFIG_IKCONFIG is not set CONFIG_LOG_BUF_SHIFT=18 CONFIG_LOG_CPU_MAX_BUF_SHIFT=12 CONFIG_PRINTK_SAFE_LOG_BUF_SHIFT=13 # CONFIG_CGROUPS is not set CONFIG_NAMESPACES=y CONFIG_UTS_NS=y CONFIG_IPC_NS=y # CONFIG_USER_NS is not set CONFIG_PID_NS=y CONFIG_NET_NS=y # CONFIG_CHECKPOINT_RESTORE is not set # CONFIG_SCHED_AUTOGROUP is not set # CONFIG_SYSFS_DEPRECATED is not set CONFIG_RELAY=y CONFIG_BLK_DEV_INITRD=y CONFIG_INITRAMFS_SOURCE="" CONFIG_RD_GZIP=y CONFIG_RD_BZIP2=y CONFIG_RD_LZMA=y CONFIG_RD_XZ=y CONFIG_RD_LZO=y CONFIG_RD_LZ4=y CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE=y # CONFIG_CC_OPTIMIZE_FOR_SIZE is not set CONFIG_SYSCTL=y CONFIG_HAVE_UID16=y CONFIG_SYSCTL_EXCEPTION_TRACE=y CONFIG_BPF=y CONFIG_EXPERT=y CONFIG_UID16=y CONFIG_MULTIUSER=y CONFIG_SGETMASK_SYSCALL=y CONFIG_SYSFS_SYSCALL=y # CONFIG_SYSCTL_SYSCALL is not set CONFIG_FHANDLE=y CONFIG_POSIX_TIMERS=y CONFIG_PRINTK=y CONFIG_PRINTK_NMI=y CONFIG_BUG=y CONFIG_ELF_CORE=y CONFIG_BASE_FULL=y CONFIG_FUTEX=y CONFIG_FUTEX_PI=y CONFIG_EPOLL=y CONFIG_SIGNALFD=y CONFIG_TIMERFD=y CONFIG_EVENTFD=y CONFIG_SHMEM=y CONFIG_AIO=y CONFIG_ADVISE_SYSCALLS=y CONFIG_MEMBARRIER=y CONFIG_KALLSYMS=y # CONFIG_KALLSYMS_ALL is not set CONFIG_KALLSYMS_BASE_RELATIVE=y # CONFIG_BPF_SYSCALL is not set # CONFIG_USERFAULTFD is not set # CONFIG_EMBEDDED is not set CONFIG_HAVE_PERF_EVENTS=y CONFIG_PERF_USE_VMALLOC=y # CONFIG_PC104 is not set # # Kernel Performance Events And Counters # # CONFIG_PERF_EVENTS is not set CONFIG_VM_EVENT_COUNTERS=y # CONFIG_COMPAT_BRK is not set CONFIG_SLAB=y # CONFIG_SLUB is not set # CONFIG_SLOB is not set CONFIG_SLAB_MERGE_DEFAULT=y # CONFIG_SLAB_FREELIST_RANDOM is not set CONFIG_PROFILING=y CONFIG_TRACEPOINTS=y CONFIG_64BIT=y CONFIG_SPARC=y CONFIG_SPARC64=y CONFIG_ARCH_DEFCONFIG="arch/sparc/configs/sparc64_defconfig" CONFIG_ARCH_PROC_KCORE_TEXT=y
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 04:05, Stephen Rothwell wrote: > Hi Kirill, > > On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: >> >> This patch kills all CONFIG_SRCU defines and >> the code under !CONFIG_SRCU. >> >> Signed-off-by: Kirill Tkhai >> --- >> drivers/base/core.c| 42 >> >> include/linux/device.h |2 - >> include/linux/rcutiny.h|4 -- >> include/linux/srcu.h |5 -- >> kernel/notifier.c |3 - >> kernel/rcu/Kconfig | 12 +- >> kernel/rcu/tree.h |5 -- >> kernel/rcu/update.c|4 -- >> .../selftests/rcutorture/doc/TREE_RCU-kconfig.txt |5 -- >> 9 files changed, 3 insertions(+), 79 deletions(-) > > You left quite a few "select SRCU" statements scattered across Kconfig > files: > > $ git grep -l 'select SRCU' '*Kconfig*' > arch/arm/kvm/Kconfig > arch/arm64/kvm/Kconfig > arch/mips/kvm/Kconfig > arch/powerpc/kvm/Kconfig > arch/s390/kvm/Kconfig > arch/x86/Kconfig > arch/x86/kvm/Kconfig > block/Kconfig > drivers/clk/Kconfig > drivers/cpufreq/Kconfig > drivers/dax/Kconfig > drivers/devfreq/Kconfig > drivers/hwtracing/stm/Kconfig > drivers/md/Kconfig > drivers/net/Kconfig > drivers/opp/Kconfig > fs/btrfs/Kconfig > fs/notify/Kconfig > fs/quota/Kconfig > init/Kconfig > kernel/rcu/Kconfig > kernel/rcu/Kconfig.debug > mm/Kconfig > security/tomoyo/Kconfig Yeah, thanks, Stephen.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On 08.08.2018 04:05, Stephen Rothwell wrote: > Hi Kirill, > > On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: >> >> This patch kills all CONFIG_SRCU defines and >> the code under !CONFIG_SRCU. >> >> Signed-off-by: Kirill Tkhai >> --- >> drivers/base/core.c| 42 >> >> include/linux/device.h |2 - >> include/linux/rcutiny.h|4 -- >> include/linux/srcu.h |5 -- >> kernel/notifier.c |3 - >> kernel/rcu/Kconfig | 12 +- >> kernel/rcu/tree.h |5 -- >> kernel/rcu/update.c|4 -- >> .../selftests/rcutorture/doc/TREE_RCU-kconfig.txt |5 -- >> 9 files changed, 3 insertions(+), 79 deletions(-) > > You left quite a few "select SRCU" statements scattered across Kconfig > files: > > $ git grep -l 'select SRCU' '*Kconfig*' > arch/arm/kvm/Kconfig > arch/arm64/kvm/Kconfig > arch/mips/kvm/Kconfig > arch/powerpc/kvm/Kconfig > arch/s390/kvm/Kconfig > arch/x86/Kconfig > arch/x86/kvm/Kconfig > block/Kconfig > drivers/clk/Kconfig > drivers/cpufreq/Kconfig > drivers/dax/Kconfig > drivers/devfreq/Kconfig > drivers/hwtracing/stm/Kconfig > drivers/md/Kconfig > drivers/net/Kconfig > drivers/opp/Kconfig > fs/btrfs/Kconfig > fs/notify/Kconfig > fs/quota/Kconfig > init/Kconfig > kernel/rcu/Kconfig > kernel/rcu/Kconfig.debug > mm/Kconfig > security/tomoyo/Kconfig Yeah, thanks, Stephen.
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. The last time somebody tried to do this there was a pushback due to kernel tinyfication. So this should really give some numbers about the code size increase. Also why can't we make this depend on MMU. Is anybody else than the reclaim asking for unconditional SRCU usage? Btw. I totaly agree with Steven. This is a very poor changelog. It is trivial to see what the patch does but it is far from clear why it is doing that and why we cannot go other ways. -- Michal Hocko SUSE Labs
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Tue 07-08-18 18:37:36, Kirill Tkhai wrote: > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. The last time somebody tried to do this there was a pushback due to kernel tinyfication. So this should really give some numbers about the code size increase. Also why can't we make this depend on MMU. Is anybody else than the reclaim asking for unconditional SRCU usage? Btw. I totaly agree with Steven. This is a very poor changelog. It is trivial to see what the patch does but it is far from clear why it is doing that and why we cannot go other ways. -- Michal Hocko SUSE Labs
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
Hi Kirill, On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: > > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. > > Signed-off-by: Kirill Tkhai So what sort of overheads (in terms of code size and performance) are we adding by having SRCU enabled where it used not to be? -- Cheers, Stephen Rothwell pgpmgfAMS12n9.pgp Description: OpenPGP digital signature
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
Hi Kirill, On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: > > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. > > Signed-off-by: Kirill Tkhai So what sort of overheads (in terms of code size and performance) are we adding by having SRCU enabled where it used not to be? -- Cheers, Stephen Rothwell pgpmgfAMS12n9.pgp Description: OpenPGP digital signature
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
Hi Kirill, On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: > > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. > > Signed-off-by: Kirill Tkhai > --- > drivers/base/core.c| 42 > > include/linux/device.h |2 - > include/linux/rcutiny.h|4 -- > include/linux/srcu.h |5 -- > kernel/notifier.c |3 - > kernel/rcu/Kconfig | 12 +- > kernel/rcu/tree.h |5 -- > kernel/rcu/update.c|4 -- > .../selftests/rcutorture/doc/TREE_RCU-kconfig.txt |5 -- > 9 files changed, 3 insertions(+), 79 deletions(-) You left quite a few "select SRCU" statements scattered across Kconfig files: $ git grep -l 'select SRCU' '*Kconfig*' arch/arm/kvm/Kconfig arch/arm64/kvm/Kconfig arch/mips/kvm/Kconfig arch/powerpc/kvm/Kconfig arch/s390/kvm/Kconfig arch/x86/Kconfig arch/x86/kvm/Kconfig block/Kconfig drivers/clk/Kconfig drivers/cpufreq/Kconfig drivers/dax/Kconfig drivers/devfreq/Kconfig drivers/hwtracing/stm/Kconfig drivers/md/Kconfig drivers/net/Kconfig drivers/opp/Kconfig fs/btrfs/Kconfig fs/notify/Kconfig fs/quota/Kconfig init/Kconfig kernel/rcu/Kconfig kernel/rcu/Kconfig.debug mm/Kconfig security/tomoyo/Kconfig -- Cheers, Stephen Rothwell pgpCG1XaJwJMj.pgp Description: OpenPGP digital signature
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
Hi Kirill, On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: > > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. > > Signed-off-by: Kirill Tkhai > --- > drivers/base/core.c| 42 > > include/linux/device.h |2 - > include/linux/rcutiny.h|4 -- > include/linux/srcu.h |5 -- > kernel/notifier.c |3 - > kernel/rcu/Kconfig | 12 +- > kernel/rcu/tree.h |5 -- > kernel/rcu/update.c|4 -- > .../selftests/rcutorture/doc/TREE_RCU-kconfig.txt |5 -- > 9 files changed, 3 insertions(+), 79 deletions(-) You left quite a few "select SRCU" statements scattered across Kconfig files: $ git grep -l 'select SRCU' '*Kconfig*' arch/arm/kvm/Kconfig arch/arm64/kvm/Kconfig arch/mips/kvm/Kconfig arch/powerpc/kvm/Kconfig arch/s390/kvm/Kconfig arch/x86/Kconfig arch/x86/kvm/Kconfig block/Kconfig drivers/clk/Kconfig drivers/cpufreq/Kconfig drivers/dax/Kconfig drivers/devfreq/Kconfig drivers/hwtracing/stm/Kconfig drivers/md/Kconfig drivers/net/Kconfig drivers/opp/Kconfig fs/btrfs/Kconfig fs/notify/Kconfig fs/quota/Kconfig init/Kconfig kernel/rcu/Kconfig kernel/rcu/Kconfig.debug mm/Kconfig security/tomoyo/Kconfig -- Cheers, Stephen Rothwell pgpCG1XaJwJMj.pgp Description: OpenPGP digital signature
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. Can you add the rationale for removing the SRCU config in the change log please. Thanks! -- Steve > > Signed-off-by: Kirill Tkhai > --- > drivers/base/core.c| 42 > > include/linux/device.h |2 - > include/linux/rcutiny.h|4 -- > include/linux/srcu.h |5 -- > kernel/notifier.c |3 - > kernel/rcu/Kconfig | 12 +- > kernel/rcu/tree.h |5 -- > kernel/rcu/update.c|4 -- > .../selftests/rcutorture/doc/TREE_RCU-kconfig.txt |5 -- > 9 files changed, 3 insertions(+), 79 deletions(-) > >
Re: [PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
On Tue, 07 Aug 2018 18:37:36 +0300 Kirill Tkhai wrote: > This patch kills all CONFIG_SRCU defines and > the code under !CONFIG_SRCU. Can you add the rationale for removing the SRCU config in the change log please. Thanks! -- Steve > > Signed-off-by: Kirill Tkhai > --- > drivers/base/core.c| 42 > > include/linux/device.h |2 - > include/linux/rcutiny.h|4 -- > include/linux/srcu.h |5 -- > kernel/notifier.c |3 - > kernel/rcu/Kconfig | 12 +- > kernel/rcu/tree.h |5 -- > kernel/rcu/update.c|4 -- > .../selftests/rcutorture/doc/TREE_RCU-kconfig.txt |5 -- > 9 files changed, 3 insertions(+), 79 deletions(-) > >
[PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
This patch kills all CONFIG_SRCU defines and
the code under !CONFIG_SRCU.
Signed-off-by: Kirill Tkhai
---
drivers/base/core.c| 42
include/linux/device.h |2 -
include/linux/rcutiny.h|4 --
include/linux/srcu.h |5 --
kernel/notifier.c |3 -
kernel/rcu/Kconfig | 12 +-
kernel/rcu/tree.h |5 --
kernel/rcu/update.c|4 --
.../selftests/rcutorture/doc/TREE_RCU-kconfig.txt |5 --
9 files changed, 3 insertions(+), 79 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd779e11..8483da53c88f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -44,7 +44,6 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
/* Device links support. */
-#ifdef CONFIG_SRCU
static DEFINE_MUTEX(device_links_lock);
DEFINE_STATIC_SRCU(device_links_srcu);
@@ -67,30 +66,6 @@ void device_links_read_unlock(int idx)
{
srcu_read_unlock(_links_srcu, idx);
}
-#else /* !CONFIG_SRCU */
-static DECLARE_RWSEM(device_links_lock);
-
-static inline void device_links_write_lock(void)
-{
- down_write(_links_lock);
-}
-
-static inline void device_links_write_unlock(void)
-{
- up_write(_links_lock);
-}
-
-int device_links_read_lock(void)
-{
- down_read(_links_lock);
- return 0;
-}
-
-void device_links_read_unlock(int not_used)
-{
- up_read(_links_lock);
-}
-#endif /* !CONFIG_SRCU */
/**
* device_is_dependent - Check if one device depends on another one
@@ -317,7 +292,6 @@ static void device_link_free(struct device_link *link)
kfree(link);
}
-#ifdef CONFIG_SRCU
static void __device_link_free_srcu(struct rcu_head *rhead)
{
device_link_free(container_of(rhead, struct device_link, rcu_head));
@@ -337,22 +311,6 @@ static void __device_link_del(struct kref *kref)
list_del_rcu(>c_node);
call_srcu(_links_srcu, >rcu_head, __device_link_free_srcu);
}
-#else /* !CONFIG_SRCU */
-static void __device_link_del(struct kref *kref)
-{
- struct device_link *link = container_of(kref, struct device_link, kref);
-
- dev_info(link->consumer, "Dropping the link to %s\n",
-dev_name(link->supplier));
-
- if (link->flags & DL_FLAG_PM_RUNTIME)
- pm_runtime_drop_link(link->consumer);
-
- list_del(>s_node);
- list_del(>c_node);
- device_link_free(link);
-}
-#endif /* !CONFIG_SRCU */
/**
* device_link_del - Delete a link between two devices.
diff --git a/include/linux/device.h b/include/linux/device.h
index 8f882549edee..524dc17d67be 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -827,9 +827,7 @@ struct device_link {
u32 flags;
bool rpm_active;
struct kref kref;
-#ifdef CONFIG_SRCU
struct rcu_head rcu_head;
-#endif
};
/**
diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
index 8d9a0ea8f0b5..63e2b6f2e94a 100644
--- a/include/linux/rcutiny.h
+++ b/include/linux/rcutiny.h
@@ -115,11 +115,7 @@ static inline void rcu_irq_exit_irqson(void) { }
static inline void rcu_irq_enter_irqson(void) { }
static inline void rcu_irq_exit(void) { }
static inline void exit_rcu(void) { }
-#ifdef CONFIG_SRCU
void rcu_scheduler_starting(void);
-#else /* #ifndef CONFIG_SRCU */
-static inline void rcu_scheduler_starting(void) { }
-#endif /* #else #ifndef CONFIG_SRCU */
static inline void rcu_end_inkernel_boot(void) { }
static inline bool rcu_is_watching(void) { return true; }
diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 3e72a291c401..27238223a78e 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -60,11 +60,8 @@ int init_srcu_struct(struct srcu_struct *sp);
#include
#elif defined(CONFIG_TREE_SRCU)
#include
-#elif defined(CONFIG_SRCU)
-#error "Unknown SRCU implementation specified to kernel configuration"
#else
-/* Dummy definition for things like notifiers. Actual use gets link error. */
-struct srcu_struct { };
+#error "Unknown SRCU implementation specified to kernel configuration"
#endif
void call_srcu(struct srcu_struct *sp, struct rcu_head *head,
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 6196af8a8223..6e4b55e74736 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -402,7 +402,6 @@ int raw_notifier_call_chain(struct raw_notifier_head *nh,
}
EXPORT_SYMBOL_GPL(raw_notifier_call_chain);
-#ifdef CONFIG_SRCU
/*
* SRCU notifier chain routines.Registration and unregistration
* use a mutex, and call_chain is synchronized by SRCU (no locks).
@@ -529,8 +528,6 @@ void srcu_init_notifier_head(struct srcu_notifier_head *nh)
}
EXPORT_SYMBOL_GPL(srcu_init_notifier_head);
-#endif /* CONFIG_SRCU */
-
static ATOMIC_NOTIFIER_HEAD(die_chain);
int
[PATCH RFC 01/10] rcu: Make CONFIG_SRCU unconditionally enabled
This patch kills all CONFIG_SRCU defines and
the code under !CONFIG_SRCU.
Signed-off-by: Kirill Tkhai
---
drivers/base/core.c| 42
include/linux/device.h |2 -
include/linux/rcutiny.h|4 --
include/linux/srcu.h |5 --
kernel/notifier.c |3 -
kernel/rcu/Kconfig | 12 +-
kernel/rcu/tree.h |5 --
kernel/rcu/update.c|4 --
.../selftests/rcutorture/doc/TREE_RCU-kconfig.txt |5 --
9 files changed, 3 insertions(+), 79 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 04bbcd779e11..8483da53c88f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -44,7 +44,6 @@ early_param("sysfs.deprecated", sysfs_deprecated_setup);
/* Device links support. */
-#ifdef CONFIG_SRCU
static DEFINE_MUTEX(device_links_lock);
DEFINE_STATIC_SRCU(device_links_srcu);
@@ -67,30 +66,6 @@ void device_links_read_unlock(int idx)
{
srcu_read_unlock(_links_srcu, idx);
}
-#else /* !CONFIG_SRCU */
-static DECLARE_RWSEM(device_links_lock);
-
-static inline void device_links_write_lock(void)
-{
- down_write(_links_lock);
-}
-
-static inline void device_links_write_unlock(void)
-{
- up_write(_links_lock);
-}
-
-int device_links_read_lock(void)
-{
- down_read(_links_lock);
- return 0;
-}
-
-void device_links_read_unlock(int not_used)
-{
- up_read(_links_lock);
-}
-#endif /* !CONFIG_SRCU */
/**
* device_is_dependent - Check if one device depends on another one
@@ -317,7 +292,6 @@ static void device_link_free(struct device_link *link)
kfree(link);
}
-#ifdef CONFIG_SRCU
static void __device_link_free_srcu(struct rcu_head *rhead)
{
device_link_free(container_of(rhead, struct device_link, rcu_head));
@@ -337,22 +311,6 @@ static void __device_link_del(struct kref *kref)
list_del_rcu(>c_node);
call_srcu(_links_srcu, >rcu_head, __device_link_free_srcu);
}
-#else /* !CONFIG_SRCU */
-static void __device_link_del(struct kref *kref)
-{
- struct device_link *link = container_of(kref, struct device_link, kref);
-
- dev_info(link->consumer, "Dropping the link to %s\n",
-dev_name(link->supplier));
-
- if (link->flags & DL_FLAG_PM_RUNTIME)
- pm_runtime_drop_link(link->consumer);
-
- list_del(>s_node);
- list_del(>c_node);
- device_link_free(link);
-}
-#endif /* !CONFIG_SRCU */
/**
* device_link_del - Delete a link between two devices.
diff --git a/include/linux/device.h b/include/linux/device.h
index 8f882549edee..524dc17d67be 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -827,9 +827,7 @@ struct device_link {
u32 flags;
bool rpm_active;
struct kref kref;
-#ifdef CONFIG_SRCU
struct rcu_head rcu_head;
-#endif
};
/**
diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
index 8d9a0ea8f0b5..63e2b6f2e94a 100644
--- a/include/linux/rcutiny.h
+++ b/include/linux/rcutiny.h
@@ -115,11 +115,7 @@ static inline void rcu_irq_exit_irqson(void) { }
static inline void rcu_irq_enter_irqson(void) { }
static inline void rcu_irq_exit(void) { }
static inline void exit_rcu(void) { }
-#ifdef CONFIG_SRCU
void rcu_scheduler_starting(void);
-#else /* #ifndef CONFIG_SRCU */
-static inline void rcu_scheduler_starting(void) { }
-#endif /* #else #ifndef CONFIG_SRCU */
static inline void rcu_end_inkernel_boot(void) { }
static inline bool rcu_is_watching(void) { return true; }
diff --git a/include/linux/srcu.h b/include/linux/srcu.h
index 3e72a291c401..27238223a78e 100644
--- a/include/linux/srcu.h
+++ b/include/linux/srcu.h
@@ -60,11 +60,8 @@ int init_srcu_struct(struct srcu_struct *sp);
#include
#elif defined(CONFIG_TREE_SRCU)
#include
-#elif defined(CONFIG_SRCU)
-#error "Unknown SRCU implementation specified to kernel configuration"
#else
-/* Dummy definition for things like notifiers. Actual use gets link error. */
-struct srcu_struct { };
+#error "Unknown SRCU implementation specified to kernel configuration"
#endif
void call_srcu(struct srcu_struct *sp, struct rcu_head *head,
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 6196af8a8223..6e4b55e74736 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -402,7 +402,6 @@ int raw_notifier_call_chain(struct raw_notifier_head *nh,
}
EXPORT_SYMBOL_GPL(raw_notifier_call_chain);
-#ifdef CONFIG_SRCU
/*
* SRCU notifier chain routines.Registration and unregistration
* use a mutex, and call_chain is synchronized by SRCU (no locks).
@@ -529,8 +528,6 @@ void srcu_init_notifier_head(struct srcu_notifier_head *nh)
}
EXPORT_SYMBOL_GPL(srcu_init_notifier_head);
-#endif /* CONFIG_SRCU */
-
static ATOMIC_NOTIFIER_HEAD(die_chain);
int
