| Message ID | 20180801215759.103758-1-ndesaulniers@google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | inet/connection_sock: prefer _THIS_IP_ to current_text_addr | expand |
From: Nick Desaulniers <ndesaulniers@google.com> Date: Wed, 1 Aug 2018 14:57:59 -0700 > - sk, what, when, current_text_addr()); > + sk, what, when, (void *)_THIS_IP_); Is a fugly macro in all caps really better than a function()? I'm really surprised that _THIS_IP_ is being chosen over current_text_addr(), seriously.
On Wed, Aug 01, 2018 at 06:42:08PM -0700, David Miller wrote: > From: Nick Desaulniers <ndesaulniers@google.com> > Date: Wed, 1 Aug 2018 14:57:59 -0700 > > > - sk, what, when, current_text_addr()); > > + sk, what, when, (void *)_THIS_IP_); > > Is a fugly macro in all caps really better than a function()? > > I'm really surprised that _THIS_IP_ is being chosen over > current_text_addr(), seriously. Hi Dave, current_text_addr is only used in five places in the entire kernel and this is the only non-architecture specific use. include/net/inet_connection_sock.h:227: sk, what, when, current_text_addr()); arch/sh/include/asm/kexec.h:64: newregs->pc = (unsigned long)current_text_addr(); arch/sh/kernel/dwarf.c:602: pc = (unsigned long)current_text_addr(); arch/x86/include/asm/kexec.h:135: newregs->ip = (unsigned long)current_text_addr(); arch/parisc/kernel/unwind.c:442: r.iaoq[0] = (unsigned long) current_text_addr(); We're trying to merge it with _THIS_IP_ before for most architectures, it is defined nearly identically to _THIS_IP_: #define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; }) #define current_text_addr() ({ __label__ _l; _l: &&_l; }) * arc * arm * arm64 * h8300 * hexagon * m68k * mips * nds32 * nios2 * openrisc * powerpc * riscv * unicore32 * xtensa Ultimately, we're trying to turn off a new Clang warning in _THIS_IP_ which requires some changes to work around what is believed to be a GCC bug (see https://lore.kernel.org/patchwork/patch/969101/). I guess the alternative to this patch is to just define current_text_addr as _THIS_IP_ for all of those architectures, I am not sure how that filters into Nick's plan (I think the goal of this was to try and avoid getting all of the architecture folks involved). Thanks for your time! Nathan
On Wed, Aug 01, 2018 at 09:58:25PM -0700, Nathan Chancellor wrote: > On Wed, Aug 01, 2018 at 06:42:08PM -0700, David Miller wrote: > > From: Nick Desaulniers <ndesaulniers@google.com> > > Date: Wed, 1 Aug 2018 14:57:59 -0700 > > > > > - sk, what, when, current_text_addr()); > > > + sk, what, when, (void *)_THIS_IP_); > > > > Is a fugly macro in all caps really better than a function()? > > > > I'm really surprised that _THIS_IP_ is being chosen over > > current_text_addr(), seriously. > > Hi Dave, Sorry for the second email so quick after the first, needed to clarify two things. > > current_text_addr is only used in five places in the entire kernel and > this is the only non-architecture specific use. > > include/net/inet_connection_sock.h:227: sk, what, when, current_text_addr()); > arch/sh/include/asm/kexec.h:64: newregs->pc = (unsigned long)current_text_addr(); > arch/sh/kernel/dwarf.c:602: pc = (unsigned long)current_text_addr(); > arch/x86/include/asm/kexec.h:135: newregs->ip = (unsigned long)current_text_addr(); > arch/parisc/kernel/unwind.c:442: r.iaoq[0] = (unsigned long) current_text_addr(); > > We're trying to merge it with _THIS_IP_ before for most architectures, because for most... > it is defined nearly identically to _THIS_IP_: > > #define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; }) > > #define current_text_addr() ({ __label__ _l; _l: &&_l; }) > > * arc > * arm > * arm64 > * h8300 > * hexagon > * m68k > * mips > * nds32 > * nios2 > * openrisc > * powerpc > * riscv > * unicore32 > * xtensa > > Ultimately, we're trying to turn off a new Clang warning in _THIS_IP_ > which requires some changes to work around what is believed to be a GCC > bug (see https://lore.kernel.org/patchwork/patch/969101/). and I should have mentioned that current_text_addr suffers from the exact same issue so we're trying not to duplicate it 14 times across the kernel. > I guess the alternative to this patch is to just define current_text_addr > as _THIS_IP_ for all of those architectures, I am not sure how that > filters into Nick's plan (I think the goal of this was to try and avoid > getting all of the architecture folks involved). > > Thanks for your time! > Nathan
On Wed, Aug 1, 2018 at 6:42 PM David Miller <davem@davemloft.net> wrote: > > From: Nick Desaulniers <ndesaulniers@google.com> > Date: Wed, 1 Aug 2018 14:57:59 -0700 > > > - sk, what, when, current_text_addr()); > > + sk, what, when, (void *)_THIS_IP_); > > Is a fugly macro in all caps really better than a function()? > > I'm really surprised that _THIS_IP_ is being chosen over > current_text_addr(), seriously. See: https://lkml.org/lkml/2018/8/1/1689 I'm happy to rename it on your suggestion, after we've consolidated them.
From: Nick Desaulniers <ndesaulniers@google.com> Date: Wed, 1 Aug 2018 14:57:59 -0700 > As part of the effort to reduce the code duplication between _THIS_IP_ > and current_text_addr(), let's consolidate callers of > current_text_addr() to use _THIS_IP_. > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> I'll ACk this for now: Acked-by: David S. Miller <davem@davemloft.net>
On Thu, Aug 2, 2018 at 2:42 PM David Miller <davem@davemloft.net> wrote: > > From: Nick Desaulniers <ndesaulniers@google.com> > Date: Wed, 1 Aug 2018 14:57:59 -0700 > > > As part of the effort to reduce the code duplication between _THIS_IP_ > > and current_text_addr(), let's consolidate callers of > > current_text_addr() to use _THIS_IP_. > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > > I'll ACk this for now: > > Acked-by: David S. Miller <davem@davemloft.net> Thank you David. Should you take it into the net tree, can you please add: Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4 to the commit message? This will help us as Nathan stated previously in this thread.
From: Nick Desaulniers <ndesaulniers@google.com> Date: Thu, 2 Aug 2018 15:10:00 -0700 > On Thu, Aug 2, 2018 at 2:42 PM David Miller <davem@davemloft.net> wrote: >> >> From: Nick Desaulniers <ndesaulniers@google.com> >> Date: Wed, 1 Aug 2018 14:57:59 -0700 >> >> > As part of the effort to reduce the code duplication between _THIS_IP_ >> > and current_text_addr(), let's consolidate callers of >> > current_text_addr() to use _THIS_IP_. >> > >> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> >> >> I'll ACk this for now: >> >> Acked-by: David S. Miller <davem@davemloft.net> > > Thank you David. Should you take it into the net tree, can you please add: > > Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4 > > to the commit message? This will help us as Nathan stated previously > in this thread. Why in the world is this a stable candidate? It doesn't fix a bug. My understanding is that you are trying to do a consolidation of code and clean things up so that only one interface is used. That means it should, at best, go to the net-next tree. Thank you.
On Thu, Aug 2, 2018 at 3:17 PM David Miller <davem@davemloft.net> wrote: > > From: Nick Desaulniers <ndesaulniers@google.com> > Date: Thu, 2 Aug 2018 15:10:00 -0700 > > > On Thu, Aug 2, 2018 at 2:42 PM David Miller <davem@davemloft.net> wrote: > >> > >> From: Nick Desaulniers <ndesaulniers@google.com> > >> Date: Wed, 1 Aug 2018 14:57:59 -0700 > >> > >> > As part of the effort to reduce the code duplication between _THIS_IP_ > >> > and current_text_addr(), let's consolidate callers of > >> > current_text_addr() to use _THIS_IP_. > >> > > >> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > >> > >> I'll ACk this for now: > >> > >> Acked-by: David S. Miller <davem@davemloft.net> > > > > Thank you David. Should you take it into the net tree, can you please add: > > > > Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4 > > > > to the commit message? This will help us as Nathan stated previously > > in this thread. > > Why in the world is this a stable candidate? It doesn't fix a bug. > > My understanding is that you are trying to do a consolidation of code > and clean things up so that only one interface is used. > > That means it should, at best, go to the net-next tree. > > Thank you. + gkh We're trying to provide backports for all clang related bugs and warnings to the stable trees back through 4.4.
On Thu, Aug 2, 2018 at 3:17 PM David Miller <davem@davemloft.net> wrote: > > From: Nick Desaulniers <ndesaulniers@google.com> > Date: Thu, 2 Aug 2018 15:10:00 -0700 > > > On Thu, Aug 2, 2018 at 2:42 PM David Miller <davem@davemloft.net> wrote: > >> > >> From: Nick Desaulniers <ndesaulniers@google.com> > >> Date: Wed, 1 Aug 2018 14:57:59 -0700 > >> > >> > As part of the effort to reduce the code duplication between _THIS_IP_ > >> > and current_text_addr(), let's consolidate callers of > >> > current_text_addr() to use _THIS_IP_. > >> > > >> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > >> > >> I'll ACk this for now: > >> > >> Acked-by: David S. Miller <davem@davemloft.net> > > > > Thank you David. Should you take it into the net tree, can you please add: > > > > Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4 > > > > to the commit message? This will help us as Nathan stated previously > > in this thread. > > Why in the world is this a stable candidate? It doesn't fix a bug. > > My understanding is that you are trying to do a consolidation of code > and clean things up so that only one interface is used. > > That means it should, at best, go to the net-next tree. David, Can you please pick this up for the net-next tree then?
From: Nick Desaulniers <ndesaulniers@google.com> Date: Mon, 13 Aug 2018 14:10:08 -0700 > Can you please pick this up for the net-next tree then? Sure, done.
diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h index 0a6c9e0f2b5a..f2a87e85c07b 100644 --- a/include/net/inet_connection_sock.h +++ b/include/net/inet_connection_sock.h @@ -19,6 +19,7 @@ #include <linux/string.h> #include <linux/timer.h> #include <linux/poll.h> +#include <linux/kernel.h> #include <net/inet_sock.h> #include <net/request_sock.h> @@ -224,7 +225,7 @@ static inline void inet_csk_reset_xmit_timer(struct sock *sk, const int what, if (when > max_when) { pr_debug("reset_xmit_timer: sk=%p %d when=0x%lx, caller=%p\n", - sk, what, when, current_text_addr()); + sk, what, when, (void *)_THIS_IP_); when = max_when; }
As part of the effort to reduce the code duplication between _THIS_IP_ and current_text_addr(), let's consolidate callers of current_text_addr() to use _THIS_IP_. Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- include/net/inet_connection_sock.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)