linuxpki: correct is_{zero,broadcast}_ether_addr

[emaste]

Summary

Broadcast address is all 0xff and zero is all 0x00. Previously these functions tested the sum of the address octets (against 6 * 0xff for broadcast and 0x00 for zero), so for example fa:00:00:00:00:00 would be considered a broadcast address and fa:06:00:00:00:00 would be considered a zero address.

Sponsored by: The FreeBSD Foundation

[emaste]

meh

[cem]

Why abandon? LGTM if you still want to commit it.

[kib]

I believe that the arithmetic is performed on the type promoted as unsigned to integer, so really it is not that straightforward.

[emaste]

The functions here don't match the canonical versions in sys/net/ethernet.h and seem unusual, but aren't actually broken since there's no wrapping on the u8 additions. That said I'm still happy to commit this if it seems more standard.

[cem]

Ah, I see. Well, this change, or a comment, or like a single (int) will at least prevent someone else from scratching their head over the same thing.

[kib]

I think it is fine to commit. Phab does not allow me to accept the revision.

[• hselasky]

Hi,

If the functions are not broken there is no reason to fix them from my POV.

Please keep in mind that on amd64 there are CPU instructions which can ADD multiple variables at the same time. This is not true for AND and OR.

Further we want to differentiate from Linux a bit.

#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>

static inline bool
test_ether_addr(const uint8_t * addr)
{
        return ((addr[0] + addr[1] + addr[2] + addr[3] + addr[4] + addr[5]) == 0);
}

                                                                
int main()
{
        static uint8_t addr[6] = { 0xFc, 0x04 };

        printf("%d\n", test_ether_addr(addr));

        return (0);
}

[• hselasky]

Maybe the following is unclear:

When you compare with a signed integer the summation should also be done integer wise?

[cem]

In D21037#459021, @hselasky wrote:

If the functions are not broken there is no reason to fix them from my POV.

I think it's worth making the implicit int cast explicit, for reader clarity. This wouldn't be a semantic or target code change.

Please keep in mind that on amd64 there are CPU instructions which can ADD multiple variables at the same time. This is not true for AND and OR.

This doesn't seem to be the case, depending on SIMD extensions available / used by default. Clang 8.0.0 plain "-O3" generates vectorized code for the AND/OR variants (adding/oring multiple variables at the same time), but NOT the ADD variants: see P282. When -mno-sse is used, like with kernel code, the ADD variants do not change. The non-SIMD AND/OR variants generate slightly smaller code than the ADD variants separately. (With explicit -mavx ... flags, Clang 8.0.0 does generate vectorized ADD as well: P283.)

However, one interesting aspect of the ADD approach is that a caller which checks for both zero addresses and broadcast addresses can save the result of the add, without performing additional memory access or computation (other than cmp 0x5fa): P281. Nothing similarly clever happens when both the AND/OR variants are static inlined.

Further we want to differentiate from Linux a bit.

I don't understand this comment. Can you elaborate? IMO, we want to differentiate from Linux when and only when it is possible to do better than Linux does; not gratuitously.

[kib]

In D21037#459021, @hselasky wrote:

Please keep in mind that on amd64 there are CPU instructions which can ADD multiple variables at the same time. This is not true for AND and OR.

There is no regular (operating on the integer regiter file) instructions on amd64 that could add e.g. three registers once. There is a LEA instruction which allows you to add to registers and a constant, but thats all.

[emaste]

They are indeed not broken which is why I originally abandoned the review, but reopened it based on cem's feedback - as written there's some opportunity for confusion.

[• hselasky]

I don't understand this comment. Can you elaborate? IMO, we want to differentiate from Linux when and only when it is possible to do better than Linux does; not gratuitously.

If we can do things differently and performance is not worsened, I think that is good, to avoid people complaining we are just C&P. If we can do things better we should do that.

Your point that the result can be reused for the two functions if we use ADD is a good point.

[• hselasky]

There is no regular (operating on the integer regiter file) instructions on amd64 that could add e.g. three registers once. There is a LEA instruction which allows you to add to registers and a constant, but thats all.

I thought LEA could do better. My bad.