Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210507220813.365382-1-arnd@kernel.org> <20210507220813.365382-13-arnd@kernel.org>
 <CAHk-=wiqLPZbiWFZ3rDNCY0fm=dFR3SSDONvrVNVbkOQmQS1vw@mail.gmail.com>
In-Reply-To: <CAHk-=wiqLPZbiWFZ3rDNCY0fm=dFR3SSDONvrVNVbkOQmQS1vw@mail.gmail.com>
From:   Arnd Bergmann <arnd@kernel.org>
Date:   Sat, 8 May 2021 11:28:26 +0200
Message-ID: <CAK8P3a3sxfYG4WReXPe6fg33K7tQaP4K-F53yBcTfyEXv0W22A@mail.gmail.com>
Subject: Re: [RFC 12/12] asm-generic: simplify asm/unaligned.h
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     linux-arch <linux-arch@vger.kernel.org>,
        Vineet Gupta <vgupta@synopsys.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, May 8, 2021 at 1:54 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, May 7, 2021 at 3:12 PM Arnd Bergmann <arnd@kernel.org> wrote:
> >
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > The get_unaligned()/put_unaligned() implementations are much more complex
> > than necessary, now that all architectures use the same code.
>
> Thanks for doing this, it looks good to me.
>
> I suspect it's still slightly unnecessarily complicated - why is that
> get_unaligned() not just
>
>   #define get_unaligned(ptr) \
>         __get_unaligned_t(typeof(*__ptr), __ptr)
>
> Because I'm not seeing the reason for doing that "__auto_type __ptr"
> thing - the argument to a "typeof()" isn't actually evaluated.

Both versions are equally correct, I picked the __auto_type version
because this tends to produce smaller preprocessor output when you have
multiple layers of nested macros with 'ptr' expanding to something
complicated, and the get_unaligned() itself being expanded multiple
times again.

When I recently experimented with possible changes to cmpxchg() and
get_user(), it had a measurable impact on compile time with clang on
those macros.

get_unaligned() doesn't appear to be used much in nested macros
at all, so it probably won't actually help here, and I can just do the
simpler version instead.

I forgot to mention in the changelog that this version does not actually
require the argument to be a scalar, not sure if this is something
we want or not. It does allow developers to write something like

__be32 get_ip_saddr(struct sk_buff *skb)
{
      struct iphdr *iph = ip_hdr(skb);
      return get_unaligned(iph).saddr;
}

and get the expected result. While this seems handy, it also makes it
harder to change the macro back to one that only works on scalars
after such usage becomes widespread.

        Arnd