| Message ID | 20231024134637.3120277-29-surenb@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Memory allocation profiling | expand |
On Tue, Oct 24 2023 at 06:46, Suren Baghdasaryan wrote: > From: Kent Overstreet <kent.overstreet@linux.dev> > > This avoids a circular header dependency in an upcoming patch by only > making hrtimer.h depend on percpu-defs.h What's the actual dependency problem?
On Wed, Oct 25, 2023 at 5:33 PM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Tue, Oct 24 2023 at 06:46, Suren Baghdasaryan wrote: > > From: Kent Overstreet <kent.overstreet@linux.dev> > > > > This avoids a circular header dependency in an upcoming patch by only > > making hrtimer.h depend on percpu-defs.h > > What's the actual dependency problem? Sorry for the delay. When we instrument per-cpu allocations in [1] we need to include sched.h in percpu.h to be able to use alloc_tag_save(). sched.h includes hrtimer.h. So, without this change we end up with a circular inclusion: percpu.h->sched.h->hrtimer.h->percpu.h [1] https://lore.kernel.org/all/20231024134637.3120277-32-surenb@google.com/ >
On Thu, Oct 26 2023 at 18:33, Suren Baghdasaryan wrote: > On Wed, Oct 25, 2023 at 5:33 PM Thomas Gleixner <tglx@linutronix.de> wrote: >> > This avoids a circular header dependency in an upcoming patch by only >> > making hrtimer.h depend on percpu-defs.h >> >> What's the actual dependency problem? > > Sorry for the delay. > When we instrument per-cpu allocations in [1] we need to include > sched.h in percpu.h to be able to use alloc_tag_save(). sched.h Including sched.h in percpu.h is fundamentally wrong as sched.h is the initial place of all header recursions. There is a reason why a lot of funtionalitiy has been split out of sched.h into seperate headers over time in order to avoid that. Thanks, tglx
On Fri, Oct 27, 2023 at 01:05:48AM +0200, Thomas Gleixner wrote: > On Thu, Oct 26 2023 at 18:33, Suren Baghdasaryan wrote: > > On Wed, Oct 25, 2023 at 5:33 PM Thomas Gleixner <tglx@linutronix.de> wrote: > >> > This avoids a circular header dependency in an upcoming patch by only > >> > making hrtimer.h depend on percpu-defs.h > >> > >> What's the actual dependency problem? > > > > Sorry for the delay. > > When we instrument per-cpu allocations in [1] we need to include > > sched.h in percpu.h to be able to use alloc_tag_save(). sched.h > > Including sched.h in percpu.h is fundamentally wrong as sched.h is the > initial place of all header recursions. > > There is a reason why a lot of funtionalitiy has been split out of > sched.h into seperate headers over time in order to avoid that. Yeah, it's definitely unfortunate. The issue here is that alloc_tag_save() needs task_struct - we have to pull that in for alloc_tag_save() to be inline, which we really want. What if we moved task_struct to its own dedicated header? That might be good to do anyways...
On Fri, Oct 27, 2023, at 01:54, Kent Overstreet wrote: > On Fri, Oct 27, 2023 at 01:05:48AM +0200, Thomas Gleixner wrote: >> On Thu, Oct 26 2023 at 18:33, Suren Baghdasaryan wrote: >> > On Wed, Oct 25, 2023 at 5:33 PM Thomas Gleixner <tglx@linutronix.de> wrote: >> >> > This avoids a circular header dependency in an upcoming patch by only >> >> > making hrtimer.h depend on percpu-defs.h >> >> >> >> What's the actual dependency problem? >> > >> > Sorry for the delay. >> > When we instrument per-cpu allocations in [1] we need to include >> > sched.h in percpu.h to be able to use alloc_tag_save(). sched.h >> >> Including sched.h in percpu.h is fundamentally wrong as sched.h is the >> initial place of all header recursions. >> >> There is a reason why a lot of funtionalitiy has been split out of >> sched.h into seperate headers over time in order to avoid that. > > Yeah, it's definitely unfortunate. The issue here is that > alloc_tag_save() needs task_struct - we have to pull that in for > alloc_tag_save() to be inline, which we really want. > > What if we moved task_struct to its own dedicated header? That might be > good to do anyways... Yes, I agree that is the best way to handle it. I've prototyped a more thorough header cleanup with good results (much improved build speed) in the past, and most of the work to get there is to seperate out structures like task_struct, mm_struct, net_device, etc into headers that only depend on the embedded structure definitions without needing all the inline functions associated with them. Arnd
On Thu, Oct 26, 2023 at 11:35 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, Oct 27, 2023, at 01:54, Kent Overstreet wrote: > > On Fri, Oct 27, 2023 at 01:05:48AM +0200, Thomas Gleixner wrote: > >> On Thu, Oct 26 2023 at 18:33, Suren Baghdasaryan wrote: > >> > On Wed, Oct 25, 2023 at 5:33 PM Thomas Gleixner <tglx@linutronix.de> wrote: > >> >> > This avoids a circular header dependency in an upcoming patch by only > >> >> > making hrtimer.h depend on percpu-defs.h > >> >> > >> >> What's the actual dependency problem? > >> > > >> > Sorry for the delay. > >> > When we instrument per-cpu allocations in [1] we need to include > >> > sched.h in percpu.h to be able to use alloc_tag_save(). sched.h > >> > >> Including sched.h in percpu.h is fundamentally wrong as sched.h is the > >> initial place of all header recursions. > >> > >> There is a reason why a lot of funtionalitiy has been split out of > >> sched.h into seperate headers over time in order to avoid that. > > > > Yeah, it's definitely unfortunate. The issue here is that > > alloc_tag_save() needs task_struct - we have to pull that in for > > alloc_tag_save() to be inline, which we really want. > > > > What if we moved task_struct to its own dedicated header? That might be > > good to do anyways... > > Yes, I agree that is the best way to handle it. I've prototyped > a more thorough header cleanup with good results (much improved > build speed) in the past, and most of the work to get there is > to seperate out structures like task_struct, mm_struct, net_device, > etc into headers that only depend on the embedded structure > definitions without needing all the inline functions associated > with them. This is something I'll add to our automation todos which I plan to talk about at plumbers; I feel like it should be possible to write a script that given a header and identifier can split whatever declaration out into a new header, update the old header, then add the necessary includes for the newly created header to each dependent (optional).
diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h index 0ee140176f10..e67349e84364 100644 --- a/include/linux/hrtimer.h +++ b/include/linux/hrtimer.h @@ -16,7 +16,7 @@ #include <linux/rbtree.h> #include <linux/init.h> #include <linux/list.h> -#include <linux/percpu.h> +#include <linux/percpu-defs.h> #include <linux/seqlock.h> #include <linux/timer.h> #include <linux/timerqueue.h> diff --git a/include/linux/time_namespace.h b/include/linux/time_namespace.h index 03d9c5ac01d1..a9e61120d4e3 100644 --- a/include/linux/time_namespace.h +++ b/include/linux/time_namespace.h @@ -11,6 +11,8 @@ struct user_namespace; extern struct user_namespace init_user_ns; +struct vm_area_struct; + struct timens_offsets { struct timespec64 monotonic; struct timespec64 boottime;