| Message ID | 1611225671-2917-1-git-send-email-sumit.garg@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,INTERNAL] kdb: Make memory allocations more robust | expand |
On Thu, Jan 21, 2021 at 04:11:11PM +0530, Sumit Garg wrote: > Use in_atomic() instead of in_interrupt() as the former is more > appropriate to know atomic context and moreover the later is deprecated. > > Also, use atomic allocations when kdb entry is in normal task context [1] > with interrupts disabled. This came up before for kdb patches but for this kind of commit comment consider the template below. : Please use the customary changelog style we use in the kernel: : " Current code does (A), this has a problem when (B). : We can improve this doing (C), because (D)." -- http://lkml.iu.edu/hypermail//linux/kernel/1311.1/01157.html In other words: A: Currently kdb uses in_interrupt() to determine whether it's library code has been called from the kgdb trap handler or from a saner calling context such as driver init. B: This approach is broken because... Not only is that clearer but carefully writing a description can improve your understanding of the semantics of the call. For example one we have got a nice clear summary of what kdb is using is_interrupt() for then the solution should be clearer... > > [1] $ echo g > /proc/sysrq-trigger > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > --- > kernel/debug/kdb/kdb_private.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h > index 7a4a181..40bf351 100644 > --- a/kernel/debug/kdb/kdb_private.h > +++ b/kernel/debug/kdb/kdb_private.h > @@ -231,7 +231,7 @@ extern struct task_struct *kdb_curr_task(int); > > #define kdb_task_has_cpu(p) (task_curr(p)) > > -#define GFP_KDB (in_interrupt() ? GFP_ATOMIC : GFP_KERNEL) > +#define GFP_KDB (in_atomic() || irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL) ... since now we should be able to see that in_dbg_master() is a better test for us to use here. Daniel.
On Thu, 21 Jan 2021 at 16:59, Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Thu, Jan 21, 2021 at 04:11:11PM +0530, Sumit Garg wrote: > > Use in_atomic() instead of in_interrupt() as the former is more > > appropriate to know atomic context and moreover the later is deprecated. > > > > Also, use atomic allocations when kdb entry is in normal task context [1] > > with interrupts disabled. > > This came up before for kdb patches but for this kind of commit comment > consider the template below. > > : Please use the customary changelog style we use in the kernel: > : " Current code does (A), this has a problem when (B). > : We can improve this doing (C), because (D)." > -- http://lkml.iu.edu/hypermail//linux/kernel/1311.1/01157.html > > In other words: > > A: Currently kdb uses in_interrupt() to determine whether it's library > code has been called from the kgdb trap handler or from a saner > calling context such as driver init. > > B: This approach is broken because... > > Not only is that clearer but carefully writing a description can > improve your understanding of the semantics of the call. > > For example one we have got a nice clear summary of what kdb is using > is_interrupt() for then the solution should be clearer... > Okay, I will reword the commit message accordingly. > > > > [1] $ echo g > /proc/sysrq-trigger > > > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org> > > --- > > kernel/debug/kdb/kdb_private.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h > > index 7a4a181..40bf351 100644 > > --- a/kernel/debug/kdb/kdb_private.h > > +++ b/kernel/debug/kdb/kdb_private.h > > @@ -231,7 +231,7 @@ extern struct task_struct *kdb_curr_task(int); > > > > #define kdb_task_has_cpu(p) (task_curr(p)) > > > > -#define GFP_KDB (in_interrupt() ? GFP_ATOMIC : GFP_KERNEL) > > +#define GFP_KDB (in_atomic() || irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL) > > ... since now we should be able to see that in_dbg_master() is a better > test for us to use here. > Yeah it looks more appropriate. -Sumit > > Daniel.
diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h index 7a4a181..40bf351 100644 --- a/kernel/debug/kdb/kdb_private.h +++ b/kernel/debug/kdb/kdb_private.h @@ -231,7 +231,7 @@ extern struct task_struct *kdb_curr_task(int); #define kdb_task_has_cpu(p) (task_curr(p)) -#define GFP_KDB (in_interrupt() ? GFP_ATOMIC : GFP_KERNEL) +#define GFP_KDB (in_atomic() || irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL) extern void *debug_kmalloc(size_t size, gfp_t flags); extern void debug_kfree(void *);
Use in_atomic() instead of in_interrupt() as the former is more appropriate to know atomic context and moreover the later is deprecated. Also, use atomic allocations when kdb entry is in normal task context [1] with interrupts disabled. [1] $ echo g > /proc/sysrq-trigger Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- kernel/debug/kdb/kdb_private.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.7.4