Re: [PATCH 4/11] use ether_addr_equal_64bits

On Tue, Dec 31, 2013 at 7:26 AM, Emmanuel Grumbach [off-list ref] wrote:

On Tue, Dec 31, 2013 at 1:13 AM, Johannes Berg
[off-list ref] wrote:

quoted

On Mon, 2013-12-30 at 19:57 -0200, Henrique de Moraes Holschuh wrote:

quoted

On Mon, 30 Dec 2013, Johannes Berg wrote:

quoted

On Mon, 2013-12-30 at 20:58 +0100, Julia Lawall wrote:

quoted

quoted

Is there any way we could catch (sparse, or some other script?) that
struct reorganising won't break the condition needed ("within a
structure that contains at least two more bytes")?

What kind of reorganizing could happen?  Do you mean that the programmer
might do at some time in the future, or something the compiler might do?

I'm just thinking of a programmer, e.g. changing a struct like this:

 struct foo {
   u8 addr[ETH_ALEN];
-  u16 dummy;
 };

for example.

That is easily resolved by:

struct foo {
      u8 addr[ETH_ALEN];
      u16 required_padding;   /* do not remove upon pain of death */
};

Adding the u16 also changes the alignment of the whole struct. So it may
cost one additional byte _in front of_ the struct.

<asking-stupid-question-see-answer-below>
While you're at it, why not just making a new 64-bit aligned type for
Ethernet addresses, so it'll also work for
!CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?
</asking-stupid-question-see-answer-below>

quoted

That'd be a stupid waste of struct space. If anything, there should be
*only* a comment saying that at least two bytes are needed - I'd still
prefer an automated check.

Frankly I am not sure I like the patch. This flow is not a fast path

I also don't like it. To me this sounds like wasting space for nothing.
BTW, would it be that more expensive to always do a 32+16 bit
comparison?

at all. While I don't really care for the waste in iwlwifi (because
there isn't), I don't see the real point is make the code more
sensitive to changes to earn basically nothing.

Thanks to this discussion, my eye fell on:

static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2)
{
        const u16 *a = (const u16 *) addr1;
        const u16 *b = (const u16 *) addr2;

        BUILD_BUG_ON(ETH_ALEN != 6);
        return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])) != 0;
}

What if addr1 or addr2 are odd, and this is running on an architecture that
doesn't support unaligned accesses at all?? Have we been lucky forever?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds