Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 18:51:43 -0500, Christos Zoulas wrote:
> On Feb 17, 2:20am, u...@stderr.spb.ru (Valery Ushakov) wrote:
> -- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
>
> | Well, it was you who did the definion of ALIGNED_POINTER centralized
> | and overridable :)
> |
> | revision 1.400
> | date: 2012-01-25 00:03:36 +0400; author: christos; state: Exp; lines:
> +26 -1;
> | Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
> | and avoid definining them in 10 different places if not needed.
>
> If you look a bit deeper into that change, it merged many MD copies
> of this macro into one, and I needed the override for the archs that
> were different.
>
> | ALIGNED_POINTER is overriden on x86 to be always true. Surprisingly
> | it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
> | That is most likely an oversight, but that will probably require some
> | cvs archaeology to confirm. Some uses of ALIGNED_POINTER are inside
> | an __NO_STRICT_ALIGNMENT #ifdef.
>
> This is a mess as you can see. The problem is that in each case we
> need to determine if this macro is used to test alignment to determine
> access restrictions on certain architectures (most cases), or it
> is done for performance/protocol requirements.
>
> I hope that nothing falls into the second category and then we can
> use a single macro that uses a combination of __NO_STRICT_ALIGNMENT
> and __alignof() which my guess is that it did not exist at the time
> the macro was invented, and thus it used sizeof() and limited it to
> integral types.
>
> | We don't even seem to be sure about its semantics, as far as I can
> | tell (see bus space comments in my mail).
> |
> | That's even more of a reason to stop doing aimless random changes
> | without getting some kind of understanding first. The last thing we
> | need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
> | different semantics both of which are counter-intuitive to begin with
> | (and riastradh@ even had to add a verbose comment for that).
>
> This change was not aimless. I wanted to remove the multiple copies of
> the m_copyup/m_pullup code into one function. To do that I needed the
> alignment as a value, not a predicate macro (that was again copied in
> ~10 places). When I tried to centralize it, I wanted to do the minimal
> change so I would not screw it up (and I did end up screwing it up).
>
> This is a good opportunity now to clean this up even more and provide
> sane alignment macros.
We should start by at least separating the concerns. The test for
"aligned at power of two boundary" and the actual intent/purpose of
that test ("stuff needs to be aligned for us to safely do $FOO").
Then we can test the alignment check without jumping through
ridiculous hoops. That should have been done earlier for the
ALIGNED_POINTER change, but yeah, I can see how in the heat of the
moment that change was just "preserve the status quo" and that's
absolutely fine. But doing the same thing now with the alignment test
and the purpose of that alignment test conflacted once again under a
different but confusignly similar name is negligent (if anything we
will run out of half way decent names for the just-the-alignment-test
macro, b/c all of them will be loaded with some additional accidental
semantic).
-uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Wed, Feb 17, 2021 at 00:51:03 +0100, Roland Illig wrote: > 17.02.2021 00:25:10 Valery Ushakov : > > On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote: > >> Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the > >> test is for. > >> > >> I intentionally placed it between (which defines that > >> macro on x86 and some other platforms) and (which uses the > >> macro to switch between the boring "everything is correctly aligned" and > >> the more interesting formula suggested here. > > > > This is wrong on so many levels. What is even the point of a test > > that doesn't test the thing as it is actually defined and used in the > > code? > > The point of the test is to verify that the "complicated" formula > produces correct results. That's what several commits tried to > fix. If this test had been there from the beginning, none of the > wrong formulas would have passed it. That's the whole point. > > The point of the test was intentionally not to test the actual > behavior on each platform but to test the same formula, independent > from the platform, and to do this, I somehow needed access to that > formula. Testing the actually used formula per platform could be > added as another test. I just wanted to avoid the obviously wrong > formulas to go unnoticed in the code. That's the point of the test, > and that's exactly what it achieves. Therefore I don't see anything > wrong with it. The very fact that you need to undefine an unspecified macro at an unspecified time to get to the "formula" points to a problem. We shouldn't be pretending that it's not, and provide the false decorum of "oh, but it's covered with a test, so it's ok". -uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Feb 17, 2:20am, u...@stderr.spb.ru (Valery Ushakov) wrote: -- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys) | Well, it was you who did the definion of ALIGNED_POINTER centralized | and overridable :) | | revision 1.400 | date: 2012-01-25 00:03:36 +0400; author: christos; state: Exp; lines: +26 -1; | Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently, | and avoid definining them in 10 different places if not needed. If you look a bit deeper into that change, it merged many MD copies of this macro into one, and I needed the override for the archs that were different. | ALIGNED_POINTER is overriden on x86 to be always true. Surprisingly | it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT. | That is most likely an oversight, but that will probably require some | cvs archaeology to confirm. Some uses of ALIGNED_POINTER are inside | an __NO_STRICT_ALIGNMENT #ifdef. This is a mess as you can see. The problem is that in each case we need to determine if this macro is used to test alignment to determine access restrictions on certain architectures (most cases), or it is done for performance/protocol requirements. I hope that nothing falls into the second category and then we can use a single macro that uses a combination of __NO_STRICT_ALIGNMENT and __alignof() which my guess is that it did not exist at the time the macro was invented, and thus it used sizeof() and limited it to integral types. | We don't even seem to be sure about its semantics, as far as I can | tell (see bus space comments in my mail). | | That's even more of a reason to stop doing aimless random changes | without getting some kind of understanding first. The last thing we | need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly | different semantics both of which are counter-intuitive to begin with | (and riastradh@ even had to add a verbose comment for that). This change was not aimless. I wanted to remove the multiple copies of the m_copyup/m_pullup code into one function. To do that I needed the alignment as a value, not a predicate macro (that was again copied in ~10 places). When I tried to centralize it, I wanted to do the minimal change so I would not screw it up (and I did end up screwing it up). This is a good opportunity now to clean this up even more and provide sane alignment macros. christos
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
17.02.2021 00:25:10 Valery Ushakov : > On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote: >> Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the >> test is for. >> >> I intentionally placed it between (which defines that >> macro on x86 and some other platforms) and (which uses the >> macro to switch between the boring "everything is correctly aligned" and >> the more interesting formula suggested here. > > This is wrong on so many levels. What is even the point of a test > that doesn't test the thing as it is actually defined and used in the > code? The point of the test is to verify that the "complicated" formula produces correct results. That's what several commits tried to fix. If this test had been there from the beginning, none of the wrong formulas would have passed it. That's the whole point. The point of the test was intentionally not to test the actual behavior on each platform but to test the same formula, independent from the platform, and to do this, I somehow needed access to that formula. Testing the actually used formula per platform could be added as another test. I just wanted to avoid the obviously wrong formulas to go unnoticed in the code. That's the point of the test, and that's exactly what it achieves. Therefore I don't see anything wrong with it. Roland
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On 16.02.2021 23:32, Jason Thorpe wrote: On Feb 16, 2021, at 1:56 PM, Roland Illig wrote: A downside of this test is that the macro from gets only evaluated at compile time of the test. The test therefore cannot cover future updates to without being reinstalled itself. But at least for the release builds, it ensures a correct definition. You can make this get evaluated at run-time by passing in a non-constant argument. I wanted to make the test even more flexible: react to the then-current installed version of the macro in . To do that, the test would need to be compiled at runtime, requiring a C compiler on the target machine, and I'm not sure that's a valid assumption. And maybe that's something that shouldn't be done at all, updating the system header without the corresponding test suite. So it's probably fine as it is now, notwithstanding the discussion about whether the macro is needed at all in this prominent place. It surprised me though that neither Google nor GitHub found the macro name POINTER_ALIGNED_P anywhere outside the NetBSD repository. I would have expected this name to be already used by some projects, given that it is so straight-forward. Roland
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 17:53:09 -0500, Christos Zoulas wrote: > In this case "type" is a struct and we have __alignof() to handle > it, but does this give the right answer? > > Also ALIGNED_POINTER is not conditional to __NO_STRICT_ALIGNMENT and > can be overriden (the opposite goes for POINTER_ALIGNED_P) I am all > for having one macro, but how can we satisfy all the different > semantics? Well, it was you who did the definion of ALIGNED_POINTER centralized and overridable :) revision 1.400 date: 2012-01-25 00:03:36 +0400; author: christos; state: Exp; lines: +26 -1; Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently, and avoid definining them in 10 different places if not needed. ALIGNED_POINTER is overriden on x86 to be always true. Surprisingly it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT. That is most likely an oversight, but that will probably require some cvs archaeology to confirm. Some uses of ALIGNED_POINTER are inside an __NO_STRICT_ALIGNMENT #ifdef. We don't even seem to be sure about its semantics, as far as I can tell (see bus space comments in my mail). That's even more of a reason to stop doing aimless random changes without getting some kind of understanding first. The last thing we need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly different semantics both of which are counter-intuitive to begin with (and riastradh@ even had to add a verbose comment for that). -uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 23:54:46 +0100, Roland Illig wrote:
> On 16.02.2021 23:40, Valery Ushakov wrote:
> > On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
> >
> > > On 16.02.2021 19:46, Roy Marples wrote:
> > > > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > > > want rather than the bitwise test we supply, like so:
> > > >
> > > > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
> > >
> > > To make sure that this macro doesn't get broken again, I have written
> > > the attached tests. I will commit them after making sure I got the
> > > changes to distrib/sets/lists/tests/mi correct. All the rest I already
> > > tested successfully.
> >
> > I don't see any proposal to change the meaning of this macro to
> > actually require the alignment even for arches without strict
> > alignment. Does the attached test really passes on e.g. x86 where the
> > macro is always true? E.g. this one:
> >
> > > + if (POINTER_ALIGNED_P(p + 1, 2))
> > > + atf_tc_fail("p + 1 must not be aligned to 2");
>
> Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the
> test is for.
>
> I intentionally placed it between (which defines that
> macro on x86 and some other platforms) and (which uses the
> macro to switch between the boring "everything is correctly aligned" and
> the more interesting formula suggested here.
This is wrong on so many levels. What is even the point of a test
that doesn't test the thing as it is actually defined and used in the
code?
-uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On 16.02.2021 23:40, Valery Ushakov wrote:
On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
On 16.02.2021 19:46, Roy Marples wrote:
I suggest we change POINTER_ALIGNED_P to accept the alignment value we
want rather than the bitwise test we supply, like so:
#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
To make sure that this macro doesn't get broken again, I have written
the attached tests. I will commit them after making sure I got the
changes to distrib/sets/lists/tests/mi correct. All the rest I already
tested successfully.
I don't see any proposal to change the meaning of this macro to
actually require the alignment even for arches without strict
alignment. Does the attached test really passes on e.g. x86 where the
macro is always true? E.g. this one:
+ if (POINTER_ALIGNED_P(p + 1, 2))
+ atf_tc_fail("p + 1 must not be aligned to 2");
Yes, it does. That's what the "#undef __NO_STRICT_ALIGNMENT" in the
test is for.
I intentionally placed it between (which defines that
macro on x86 and some other platforms) and (which uses the
macro to switch between the boring "everything is correctly aligned" and
the more interesting formula suggested here.
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
In this case "type" is a struct and we have __alignof() to handle it, but does this give the right answer? Also ALIGNED_POINTER is not conditional to __NO_STRICT_ALIGNMENT and can be overriden (the opposite goes for POINTER_ALIGNED_P) I am all for having one macro, but how can we satisfy all the different semantics? christos signature.asc Description: Message signed with OpenPGP
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On Tue, Feb 16, 2021 at 22:56:19 +0100, Roland Illig wrote:
> On 16.02.2021 19:46, Roy Marples wrote:
> > I suggest we change POINTER_ALIGNED_P to accept the alignment value we
> > want rather than the bitwise test we supply, like so:
> >
> > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)))
>
> That's a good definition, clear, simple, obvious, without any surprises.
Now, replace the value "a" with the type "t" and change (a) to
sizeof(t) and you will get ALIGNED_POINTER from just above.
> To make sure that this macro doesn't get broken again, I have written
> the attached tests. I will commit them after making sure I got the
> changes to distrib/sets/lists/tests/mi correct. All the rest I already
> tested successfully.
I don't see any proposal to change the meaning of this macro to
actually require the alignment even for arches without strict
alignment. Does the attached test really passes on e.g. x86 where the
macro is always true? E.g. this one:
> + if (POINTER_ALIGNED_P(p + 1, 2))
> + atf_tc_fail("p + 1 must not be aligned to 2");
> Is my assumption correct that on each platform supported by NetBSD, a
> variable of type double gets aligned to a multiple of 8, even on the
> stack?
No. E.g. on sh3 doubles are 8 bytes but are 4 bytes aligned. I'm
almost sure some other ABI has that kind less strict alignment too,
but I don't remember.
> I wanted to keep the test as simple as possible, therefore I
> didn't want to call malloc just to get an aligned pointer.
You can use an ordinary array that is large enough and manually
find/construct a suitably aligned pointer value inside that array.
BUT, can we, PLEASE, stop making random breaking changes and at least
articulate first what is that that we want here?
POINTER_ALIGNED_P should have never been brought into existence in the
first place.
ALIGNED_POINTER seems to be used to define BUS_SPACE_ALIGNED_POINTER -
the real fun here is sys/arch/x86/include/bus_defs.h that defines
BUS_SPACE_ALIGNED_POINTER to be "really" aligned for BUS_SPACE_DEBUG,
which seems kinda suspicious to me. BTW, can we really conclude from
the CPU's alignment requirements to some bus alignment requirements?
But to get back to my main point, PLEASE, can we stop making random
aimless changes without prior discussion?
To quote Vonnegut, "If I had of been a observer, I would of said we
was comical."
-uwe
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
> On Feb 16, 2021, at 1:56 PM, Roland Illig wrote: > > A downside of this test is that the macro from gets only > evaluated at compile time of the test. The test therefore cannot cover > future updates to without being reinstalled itself. But > at least for the release builds, it ensures a correct definition. You can make this get evaluated at run-time by passing in a non-constant argument. -- thorpej
Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
On 16.02.2021 19:46, Roy Marples wrote:
I suggest we change POINTER_ALIGNED_P to accept the alignment value we
want rather than the bitwise test we supply, like so:
#define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1))
That's a good definition, clear, simple, obvious, without any surprises.
To make sure that this macro doesn't get broken again, I have written
the attached tests. I will commit them after making sure I got the
changes to distrib/sets/lists/tests/mi correct. All the rest I already
tested successfully.
Is my assumption correct that on each platform supported by NetBSD, a
variable of type double gets aligned to a multiple of 8, even on the
stack? I wanted to keep the test as simple as possible, therefore I
didn't want to call malloc just to get an aligned pointer.
A downside of this test is that the macro from gets only
evaluated at compile time of the test. The test therefore cannot cover
future updates to without being reinstalled itself. But
at least for the release builds, it ensures a correct definition.
Roland
Index: distrib/sets/lists/tests/mi
===
RCS file: /cvsroot/src/distrib/sets/lists/tests/mi,v
retrieving revision 1.1019
diff -u -p -r1.1019 mi
--- distrib/sets/lists/tests/mi 16 Feb 2021 09:46:24 - 1.1019
+++ distrib/sets/lists/tests/mi 16 Feb 2021 21:45:40 -
@@ -4396,6 +4396,10 @@
./usr/tests/sys/rc/h_args tests-sys-tests
compattestfile,atf
./usr/tests/sys/rc/h_simpletests-sys-tests
compattestfile,atf
./usr/tests/sys/rc/t_rc_d_cli tests-sys-tests
compattestfile,atf
+./usr/tests/sys/systests-sys-tests
compattestfile,atf
+./usr/tests/sys/sys/Atffiletests-sys-tests
compattestfile,atf
+./usr/tests/sys/sys/Kyuafile tests-sys-tests
compattestfile,atf,kyua
+./usr/tests/sys/sys/t_pointer_aligned_ptests-sys-tests
compattestfile,atf
./usr/tests/sys/x86tests-sys-tests
compattestfile,atf
./usr/tests/syscalltests-obsolete
obsolete
./usr/tests/syscall/Atffiletests-obsolete
obsolete
Index: tests/sys/Makefile
===
RCS file: /cvsroot/src/tests/sys/Makefile,v
retrieving revision 1.5
diff -u -p -r1.5 Makefile
--- tests/sys/Makefile 15 Oct 2020 17:44:44 - 1.5
+++ tests/sys/Makefile 16 Feb 2021 21:45:40 -
@@ -10,6 +10,7 @@ TESTS_SUBDIRS+= netatalk
TESTS_SUBDIRS+=netinet
TESTS_SUBDIRS+=netinet6
TESTS_SUBDIRS+=rc
+TESTS_SUBDIRS+=sys
.if ${MACHINE} == amd64 || ${MACHINE} == i386
TESTS_SUBDIRS+=x86
.endif
Index: tests/sys/sys/Makefile
===
RCS file: tests/sys/sys/Makefile
diff -N tests/sys/sys/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ tests/sys/sys/Makefile 16 Feb 2021 21:45:40 -
@@ -0,0 +1,11 @@
+# $NetBSD$
+
+TESTSDIR= ${TESTSBASE}/sys/sys
+WARNS= 6
+
+TESTS_C= t_pointer_aligned_p
+
+.PATH: ${NETBSDSRCDIR}/sys
+CPPFLAGS+= -I${NETBSDSRCDIR}/sys
+
+.include
Index: tests/sys/sys/t_pointer_aligned_p.c
===
RCS file: tests/sys/sys/t_pointer_aligned_p.c
diff -N tests/sys/sys/t_pointer_aligned_p.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ tests/sys/sys/t_pointer_aligned_p.c 16 Feb 2021 21:45:40 -
@@ -0,0 +1,77 @@
+/* $NetBSD$*/
+
+/*-
+ * Copyright (c) 2021 The NetBSD Foundation, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND
Re: CVS commit: src/sys
Yes, I agree. I am going to change that. christos > On Feb 16, 2021, at 1:46 PM, Roy Marples wrote: > > Hi Christos > > On 14/02/2021 20:58, Christos Zoulas wrote: >> Module Name: src >> Committed By:christos >> Date:Sun Feb 14 20:58:35 UTC 2021 >> Modified Files: >> src/sys/net: if_arp.h if_bridge.c >> src/sys/netinet: icmp_private.h if_arp.c igmp_var.h in_l2tp.c ip_flow.c >> ip_input.c ip_private.h tcp_input.c tcp_private.h udp_private.h >> udp_usrreq.c >> src/sys/netinet6: icmp6.c in6_l2tp.c ip6_flow.c ip6_input.c >> ip6_private.h udp6_usrreq.c >> src/sys/sys: mbuf.h param.h >> Log Message: >> - centralize header align and pullup into a single inline function >> - use a single macro to align pointers and expose the alignment, instead >> of hard-coding 3 in 1/2 the macros. >> - fix an issue in the ipv6 lt2p where it was aligning for ipv4 and pulling >> for ipv6. > > -#ifdef __NO_STRICT_ALIGNMENT > -#define IP_HDR_ALIGNED_P(ip)1 > -#else > -#define IP_HDR_ALIGNED_P(ip)vaddr_t) (ip)) & 3) == 0) > -#endif > +#define IP_HDR_ALIGNMENT3 > #endif /* _KERNEL */ > > While this is a like for like change, I feel that the meaning of > IP_HDR_ALIGNMENT is no longer clear as 3 without context makes no sense at > all. > We know it should be aligned to 4 bytes. > > I suggest we change POINTER_ALIGNED_P to accept the alignment value we want > rather than the bitwise test we supply, like so: > > #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) > == 0) > > Roy signature.asc Description: Message signed with OpenPGP
Re: CVS commit: src/sys
Hi Christos On 14/02/2021 20:58, Christos Zoulas wrote: Module Name:src Committed By: christos Date: Sun Feb 14 20:58:35 UTC 2021 Modified Files: src/sys/net: if_arp.h if_bridge.c src/sys/netinet: icmp_private.h if_arp.c igmp_var.h in_l2tp.c ip_flow.c ip_input.c ip_private.h tcp_input.c tcp_private.h udp_private.h udp_usrreq.c src/sys/netinet6: icmp6.c in6_l2tp.c ip6_flow.c ip6_input.c ip6_private.h udp6_usrreq.c src/sys/sys: mbuf.h param.h Log Message: - centralize header align and pullup into a single inline function - use a single macro to align pointers and expose the alignment, instead of hard-coding 3 in 1/2 the macros. - fix an issue in the ipv6 lt2p where it was aligning for ipv4 and pulling for ipv6. -#ifdef __NO_STRICT_ALIGNMENT -#defineIP_HDR_ALIGNED_P(ip)1 -#else -#defineIP_HDR_ALIGNED_P(ip)vaddr_t) (ip)) & 3) == 0) -#endif +#defineIP_HDR_ALIGNMENT3 #endif /* _KERNEL */ While this is a like for like change, I feel that the meaning of IP_HDR_ALIGNMENT is no longer clear as 3 without context makes no sense at all. We know it should be aligned to 4 bytes. I suggest we change POINTER_ALIGNED_P to accept the alignment value we want rather than the bitwise test we supply, like so: #define POINTER_ALIGNED_P(p, a) (((uintptr_t)(p) & ((a) - 1)) == 0) Roy
Re: CVS commit: src/sys/netinet
On Tue, Feb 16, 2021 at 09:29:15AM +, Roy Marples wrote: > In my testing on aarch64 and octeon (both of which I think are strict > alignment) neither need pullups nor copyups as the mbuf already has enough > and arphrd is aligned correctly already. Ah, we asserted too much alignment - indeed, this variant works and I commited it after testing on 32bit arm and sparc64. Martin
Re: CVS commit: src/sys/netinet
On 16/02/2021 09:20, Martin Husemann wrote:
On Tue, Feb 16, 2021 at 08:26:40AM +, Roy Marples wrote:
Is that because ARP_HDR_ALIGNMENT is forcing 4 byte alignment?
The KASSERT a few lines below triggerd, we need to be consistent.
For the purposes of using just the header we define I'm pretty sure we can
use 2 byte alignment and set ARP_HDR_ALIGNMENT to 1.
I can test (I have an alignment critical machine with non-ETHER_ALIGN'ing
network driver). Send me a patch, I lost track in the ongoing overhaul.
ARP_HDR_ALIGNED_P can now be removed from if_arp.c as well.
Not sure I understand what you mean here.
Index: net/if_arp.h
===
RCS file: /cvsroot/src/sys/net/if_arp.h,v
retrieving revision 1.40
diff -u -p -r1.40 if_arp.h
--- net/if_arp.h14 Feb 2021 20:58:34 - 1.40
+++ net/if_arp.h16 Feb 2021 09:26:23 -
@@ -72,7 +72,7 @@ structarphdr {
uint8_t ar_tpa[]; /* target protocol address */
#endif
};
-#defineARP_HDR_ALIGNMENT 3
+#defineARP_HDR_ALIGNMENT 1
static __inline uint8_t *
ar_data(struct arphdr *ap)
Index: netinet/if_arp.c
===
RCS file: /cvsroot/src/sys/netinet/if_arp.c,v
retrieving revision 1.305
diff -u -p -r1.305 if_arp.c
--- netinet/if_arp.c16 Feb 2021 05:44:13 - 1.305
+++ netinet/if_arp.c16 Feb 2021 09:26:23 -
@@ -133,12 +133,6 @@ __KERNEL_RCSID(0, "$NetBSD: if_arp.c,v 1
*/
#define ETHERTYPE_IPTRAILERS ETHERTYPE_TRAIL
-#ifdef __NO_STRICT_ALIGNMENT
-#defineARP_HDR_ALIGNED_P(ar) 1
-#else
-#defineARP_HDR_ALIGNED_P(ar) vaddr_t) (ar)) & 1) == 0)
-#endif
-
/* timers */
static int arp_reachable = REACHABLE_TIME;
static int arp_retrans = RETRANS_TIMER;
In my testing on aarch64 and octeon (both of which I think are strict alignment)
neither need pullups nor copyups as the mbuf already has enough and arphrd is
aligned correctly already.
Roy
Re: CVS commit: src/sys/netinet
On Tue, Feb 16, 2021 at 08:26:40AM +, Roy Marples wrote: > Is that because ARP_HDR_ALIGNMENT is forcing 4 byte alignment? The KASSERT a few lines below triggerd, we need to be consistent. > For the purposes of using just the header we define I'm pretty sure we can > use 2 byte alignment and set ARP_HDR_ALIGNMENT to 1. I can test (I have an alignment critical machine with non-ETHER_ALIGN'ing network driver). Send me a patch, I lost track in the ongoing overhaul. > ARP_HDR_ALIGNED_P can now be removed from if_arp.c as well. Not sure I understand what you mean here. Martin
Re: CVS commit: src/sys/netinet
On 16/02/2021 05:44, Martin Husemann wrote: Module Name:src Committed By: martin Date: Tue Feb 16 05:44:14 UTC 2021 Modified Files: src/sys/netinet: if_arp.c Log Message: Undo previous backout: alignment is needed here. The reason for the previous backout was a misunderstanding (POINTER_ALIGNED_P was broken, but the assertion fired even after it got fixed). Is that because ARP_HDR_ALIGNMENT is forcing 4 byte alignment? For the purposes of using just the header we define I'm pretty sure we can use 2 byte alignment and set ARP_HDR_ALIGNMENT to 1. ARP_HDR_ALIGNED_P can now be removed from if_arp.c as well. Roy
