Re: 2.4.17 alignment issue sparc/ia64

[Rainer Jung]

Am 16.10.2015 um 08:25 schrieb Jacob Champion:

On 10/15/2015 11:18 PM, Jacob Champion wrote:

it looks like ap_init_scoreboard() doesn't try to maintain any
particular alignment when it's assigning pointers from more_storage.


Though one would think your compiler would be padding out the struct to
a double-word multiple anyway. Hrm.


I had not yet fully read Erics mail, but the compiler would pad which 
does not help, once the stuff moves into shared memory under an address 
that is not divisible by 8. If the 64 bit struct member is then accessed 
directly in shared memory without first copying out, you would get a bus 
error.


Regards,

Rainer

Re: 2.4.17 alignment issue sparc/ia64

[Jacob Champion]

On 10/15/2015 08:53 PM, Eric Covener wrote:

We recently merged 2.4.17 and saw some bus errors on hp/ia64 and
solaris/sparc64. Selectively backing things out, it appears that the
SO_REUSEPORT patch causes the worker_score to no longer (necessarily)
be double-word aligned.


I don't have any experience with either Solaris or SPARC, but the size 
of struct process_score increased by one int with git commit 01bb7186, 
and it looks like ap_init_scoreboard() doesn't try to maintain any 
particular alignment when it's assigning pointers from more_storage.


Maybe that could be your issue? Spitballing here, but is your 
ServerLimit an odd number, and does changing it to be an even number fix 
the problem?


Hope this helps,
--Jacob

Re: 2.4.17 alignment issue sparc/ia64

[Yann Ylavic]

On Fri, Oct 16, 2015 at 5:53 AM, Eric Covener  wrote:
> We recently merged 2.4.17 and saw some bus errors on hp/ia64 and
> solaris/sparc64. Selectively backing things out, it appears that the
> SO_REUSEPORT patch causes the worker_score to no longer (necessarily)
> be double-word aligned.
>

We should do something like the following patch:

Index: server/scoreboard.c
===
--- server/scoreboard.c(revision 1708095)
+++ server/scoreboard.c(working copy)
@@ -129,14 +129,19 @@ static apr_status_t ap_cleanup_shared_mem(void *d)
 return APR_SUCCESS;
 }

+#define SIZE_OF_scoreboardAPR_ALIGN_DEFAULT(sizeof(scoreboard))
+#define SIZE_OF_global_score  APR_ALIGN_DEFAULT(sizeof(global_score))
+#define SIZE_OF_process_score APR_ALIGN_DEFAULT(sizeof(process_score))
+#define SIZE_OF_worker_score  APR_ALIGN_DEFAULT(sizeof(worker_score))
+
 AP_DECLARE(int) ap_calc_scoreboard_size(void)
 {
 ap_mpm_query(AP_MPMQ_HARD_LIMIT_THREADS, _limit);
 ap_mpm_query(AP_MPMQ_HARD_LIMIT_DAEMONS, _limit);

-scoreboard_size = sizeof(global_score);
-scoreboard_size += sizeof(process_score) * server_limit;
-scoreboard_size += sizeof(worker_score) * server_limit * thread_limit;
+scoreboard_size = SIZE_OF_global_score;
+scoreboard_size += SIZE_OF_process_score * server_limit;
+scoreboard_size += SIZE_OF_worker_score * server_limit * thread_limit;

 return scoreboard_size;
 }
@@ -153,17 +158,17 @@ AP_DECLARE(void) ap_init_scoreboard(void *shared_s

 ap_calc_scoreboard_size();
 ap_scoreboard_image =
-ap_calloc(1, sizeof(scoreboard) + server_limit *
sizeof(worker_score *));
+ap_calloc(1, SIZE_OF_scoreboard + server_limit *
sizeof(worker_score *));
 more_storage = shared_score;
 ap_scoreboard_image->global = (global_score *)more_storage;
-more_storage += sizeof(global_score);
+more_storage += SIZE_OF_global_score;
 ap_scoreboard_image->parent = (process_score *)more_storage;
-more_storage += sizeof(process_score) * server_limit;
+more_storage += SIZE_OF_process_score * server_limit;
 ap_scoreboard_image->servers =
-(worker_score **)((char*)ap_scoreboard_image + sizeof(scoreboard));
+(worker_score **)((char*)ap_scoreboard_image + SIZE_OF_scoreboard);
 for (i = 0; i < server_limit; i++) {
 ap_scoreboard_image->servers[i] = (worker_score *)more_storage;
-more_storage += thread_limit * sizeof(worker_score);
+more_storage += thread_limit * SIZE_OF_worker_score;
 }
 ap_assert(more_storage == (char*)shared_score + scoreboard_size);
 ap_scoreboard_image->global->server_limit = server_limit;
@@ -305,10 +310,10 @@ int ap_create_scoreboard(apr_pool_t *p, ap_scorebo
 if (ap_scoreboard_image) {
 ap_scoreboard_image->global->restart_time = apr_time_now();
 memset(ap_scoreboard_image->parent, 0,
-   sizeof(process_score) * server_limit);
+   SIZE_OF_process_score * server_limit);
 for (i = 0; i < server_limit; i++) {
 memset(ap_scoreboard_image->servers[i], 0,
-   sizeof(worker_score) * thread_limit);
+   SIZE_OF_worker_score * thread_limit);
 }
 ap_init_scoreboard(NULL);
 return OK;
--

Can you give it a try please?

Regards,
Yann.

Re: 2.4.17 alignment issue sparc/ia64

[Jacob Champion]

On 10/15/2015 11:18 PM, Jacob Champion wrote:

it looks like ap_init_scoreboard() doesn't try to maintain any
particular alignment when it's assigning pointers from more_storage.


Though one would think your compiler would be padding out the struct to 
a double-word multiple anyway. Hrm.


--Jacob

Re: 2.4.17 alignment issue sparc/ia64

[Jim Jagielski]

I seem to recall similar issues w/ the shm slotmem impl...

> On Oct 16, 2015, at 8:35 AM, Rainer Jung  wrote:
> 
> Am 16.10.2015 um 13:54 schrieb Yann Ylavic:
>> On Fri, Oct 16, 2015 at 10:02 AM, Yann Ylavic  wrote:
>>> 
>>> We should do something like the following patch:
>>> 
>>> Index: server/scoreboard.c
>>> ===
>>> --- server/scoreboard.c(revision 1708095)
>>> +++ server/scoreboard.c(working copy)
>>> @@ -129,14 +129,19 @@ static apr_status_t ap_cleanup_shared_mem(void *d)
>>>  return APR_SUCCESS;
>>>  }
>>> 
>>> +#define SIZE_OF_scoreboardAPR_ALIGN_DEFAULT(sizeof(scoreboard))
>>> +#define SIZE_OF_global_score  APR_ALIGN_DEFAULT(sizeof(global_score))
>>> +#define SIZE_OF_process_score APR_ALIGN_DEFAULT(sizeof(process_score))
>>> +#define SIZE_OF_worker_score  APR_ALIGN_DEFAULT(sizeof(worker_score))
>> 
>> Maybe the following would be more correct:
>> 
>> +#define SCOREBOARD_ALIGN(size) APR_ALIGN((size),sizeof(void *))
>> +#define SIZE_OF_scoreboard SCOREBOARD_ALIGN(sizeof(scoreboard))
>> +#define SIZE_OF_global_score   SCOREBOARD_ALIGN(sizeof(global_score))
>> +#define SIZE_OF_process_score  SCOREBOARD_ALIGN(sizeof(process_score))
>> +#define SIZE_OF_worker_score   SCOREBOARD_ALIGN(sizeof(worker_score))
>> 
>> since APR_ALIGN_DEFAULT seems to align to 8 bytes whatever the
>> platform requires...
>> 
>> Maybe some APR_ALIGN_NATIVE macro is missing in APR, like:
>> #define APR_ALIGN_NATIVE(size) APR_ALIGN((size), APR_SIZEOF_VOIDP)
>> but that's another story :)
> 
> I didn't yet have the time to reproduce and test your patch, but the 
> APR_ALIGN((size),sizeof(void *)) align approach would not work. The problem 
> here is that even or especially when building for 32 Bits and then using a 64 
> Bit data type, the alignment fails. So the pointer size would be 4 here, but 
> the requirement alignment 8 bytes.
> 
> Fun with hardware.
> 
> Rainer

Re: 2.4.17 alignment issue sparc/ia64

[Yann Ylavic]

On Fri, Oct 16, 2015 at 2:48 PM, Yann Ylavic  wrote:
> On Fri, Oct 16, 2015 at 2:35 PM, Rainer Jung  wrote:
>>
>> I didn't yet have the time to reproduce and test your patch, but the
>> APR_ALIGN((size),sizeof(void *)) align approach would not work. The problem
>> here is that even or especially when building for 32 Bits and then using a
>> 64 Bit data type, the alignment fails. So the pointer size would be 4 here,
>> but the requirement alignment 8 bytes.
>
> Hm, correct, each integral type (greater than the word) needs be
> aligned on its size!

Though the issue here is not accessing the type as an integral, so
aligning on 4 bytes (for 32 bit hardware) should enough for accessing
each scoreborad entry/struct (the members of the structs themselves
will still be aligned according to their type, so they can also be
safely accessed).

Re: 2.4.17 alignment issue sparc/ia64

[Yann Ylavic]

On Fri, Oct 16, 2015 at 10:02 AM, Yann Ylavic  wrote:
>
> We should do something like the following patch:
>
> Index: server/scoreboard.c
> ===
> --- server/scoreboard.c(revision 1708095)
> +++ server/scoreboard.c(working copy)
> @@ -129,14 +129,19 @@ static apr_status_t ap_cleanup_shared_mem(void *d)
>  return APR_SUCCESS;
>  }
>
> +#define SIZE_OF_scoreboardAPR_ALIGN_DEFAULT(sizeof(scoreboard))
> +#define SIZE_OF_global_score  APR_ALIGN_DEFAULT(sizeof(global_score))
> +#define SIZE_OF_process_score APR_ALIGN_DEFAULT(sizeof(process_score))
> +#define SIZE_OF_worker_score  APR_ALIGN_DEFAULT(sizeof(worker_score))

Maybe the following would be more correct:

+#define SCOREBOARD_ALIGN(size) APR_ALIGN((size),sizeof(void *))
+#define SIZE_OF_scoreboard SCOREBOARD_ALIGN(sizeof(scoreboard))
+#define SIZE_OF_global_score   SCOREBOARD_ALIGN(sizeof(global_score))
+#define SIZE_OF_process_score  SCOREBOARD_ALIGN(sizeof(process_score))
+#define SIZE_OF_worker_score   SCOREBOARD_ALIGN(sizeof(worker_score))

since APR_ALIGN_DEFAULT seems to align to 8 bytes whatever the
platform requires...

Maybe some APR_ALIGN_NATIVE macro is missing in APR, like:
#define APR_ALIGN_NATIVE(size) APR_ALIGN((size), APR_SIZEOF_VOIDP)
but that's another story :)

Re: 2.4.17 alignment issue sparc/ia64

[Rainer Jung]

Am 16.10.2015 um 13:54 schrieb Yann Ylavic:

On Fri, Oct 16, 2015 at 10:02 AM, Yann Ylavic  wrote:


We should do something like the following patch:

Index: server/scoreboard.c
===
--- server/scoreboard.c(revision 1708095)
+++ server/scoreboard.c(working copy)
@@ -129,14 +129,19 @@ static apr_status_t ap_cleanup_shared_mem(void *d)
  return APR_SUCCESS;
  }

+#define SIZE_OF_scoreboardAPR_ALIGN_DEFAULT(sizeof(scoreboard))
+#define SIZE_OF_global_score  APR_ALIGN_DEFAULT(sizeof(global_score))
+#define SIZE_OF_process_score APR_ALIGN_DEFAULT(sizeof(process_score))
+#define SIZE_OF_worker_score  APR_ALIGN_DEFAULT(sizeof(worker_score))


Maybe the following would be more correct:

+#define SCOREBOARD_ALIGN(size) APR_ALIGN((size),sizeof(void *))
+#define SIZE_OF_scoreboard SCOREBOARD_ALIGN(sizeof(scoreboard))
+#define SIZE_OF_global_score   SCOREBOARD_ALIGN(sizeof(global_score))
+#define SIZE_OF_process_score  SCOREBOARD_ALIGN(sizeof(process_score))
+#define SIZE_OF_worker_score   SCOREBOARD_ALIGN(sizeof(worker_score))

since APR_ALIGN_DEFAULT seems to align to 8 bytes whatever the
platform requires...

Maybe some APR_ALIGN_NATIVE macro is missing in APR, like:
#define APR_ALIGN_NATIVE(size) APR_ALIGN((size), APR_SIZEOF_VOIDP)
but that's another story :)


I didn't yet have the time to reproduce and test your patch, but the 
APR_ALIGN((size),sizeof(void *)) align approach would not work. The 
problem here is that even or especially when building for 32 Bits and 
then using a 64 Bit data type, the alignment fails. So the pointer size 
would be 4 here, but the requirement alignment 8 bytes.


Fun with hardware.

Rainer

Re: 2.4.17 alignment issue sparc/ia64

[Yann Ylavic]

On Fri, Oct 16, 2015 at 2:35 PM, Rainer Jung  wrote:
>
> I didn't yet have the time to reproduce and test your patch, but the
> APR_ALIGN((size),sizeof(void *)) align approach would not work. The problem
> here is that even or especially when building for 32 Bits and then using a
> 64 Bit data type, the alignment fails. So the pointer size would be 4 here,
> but the requirement alignment 8 bytes.

Hm, correct, each integral type (greater than the word) needs be
aligned on its size!

Re: 2.4.17 alignment issue sparc/ia64

[Yann Ylavic]

On Fri, Oct 16, 2015 at 3:08 PM, Rainer Jung  wrote:
> Am 16.10.2015 um 14:56 schrieb Yann Ylavic:
>>
>> On Fri, Oct 16, 2015 at 2:48 PM, Yann Ylavic  wrote:
>>>
>>> On Fri, Oct 16, 2015 at 2:35 PM, Rainer Jung 
>>> wrote:


 I didn't yet have the time to reproduce and test your patch, but the
 APR_ALIGN((size),sizeof(void *)) align approach would not work. The
 problem
 here is that even or especially when building for 32 Bits and then using
 a
 64 Bit data type, the alignment fails. So the pointer size would be 4
 here,
 but the requirement alignment 8 bytes.
>>>
>>>
>>> Hm, correct, each integral type (greater than the word) needs be
>>> aligned on its size!
>>
>>
>> Though the issue here is not accessing the type as an integral, so
>> aligning on 4 bytes (for 32 bit hardware) should enough for accessing
>> each scoreborad entry/struct (the members of the structs themselves
>> will still be aligned according to their type, so they can also be
>> safely accessed).
>
>
> Wasn't the bus error occuring in
>
> ws->last_used = apr_time_now();
>
> and the address is
>
> (dbx) print &(ws->last_used)
> >last_used = 0x7bb00094
>
> not divisible by 8 although the data type (not pointer size) is 64 Bit.

Yes but ws itself isn't aligned either:
(dbx) print ws
ws = 0x7bb00044
which is IMHO the issue.

Align ws and everything goes well (at least I think :p ).

Regards,
Yann.

Re: 2.4.17 alignment issue sparc/ia64

[Yann Ylavic]

On Fri, Oct 16, 2015 at 3:16 PM, Yann Ylavic  wrote:
> On Fri, Oct 16, 2015 at 3:08 PM, Rainer Jung  wrote:
>>
>> Wasn't the bus error occuring in
>>
>> ws->last_used = apr_time_now();
>>
>> and the address is
>>
>> (dbx) print &(ws->last_used)
>> >last_used = 0x7bb00094
>>
>> not divisible by 8 although the data type (not pointer size) is 64 Bit.
>
> Yes but ws itself isn't aligned either:
> (dbx) print ws
> ws = 0x7bb00044
> which is IMHO the issue.
>
> Align ws and everything goes well (at least I think :p ).

Hm (again), you may be right.
On sparc32 still, the compiler would do the alignment correctly for
the allocated struct if it contained a 64 bit type (forcing an 8 bytes
alignment), but we are not using malloc() here.
Since we assign the address by ourselves but don't (can't) know
whether it contains a 64bit type, we must align on 8 bytes boundary,
otherwise things like:
struct s {
apr_int64_t i64;
...
};
could fault when s->i64 is used.

That's probably why APR_ALIGN_DEFAULT(1) is always 8 which is the
largest integral type handle by the APR.
Thanks for the remainder!

Re: 2.4.17 alignment issue sparc/ia64

[Rainer Jung]

Am 16.10.2015 um 14:56 schrieb Yann Ylavic:

On Fri, Oct 16, 2015 at 2:48 PM, Yann Ylavic  wrote:

On Fri, Oct 16, 2015 at 2:35 PM, Rainer Jung  wrote:


I didn't yet have the time to reproduce and test your patch, but the
APR_ALIGN((size),sizeof(void *)) align approach would not work. The problem
here is that even or especially when building for 32 Bits and then using a
64 Bit data type, the alignment fails. So the pointer size would be 4 here,
but the requirement alignment 8 bytes.


Hm, correct, each integral type (greater than the word) needs be
aligned on its size!


Though the issue here is not accessing the type as an integral, so
aligning on 4 bytes (for 32 bit hardware) should enough for accessing
each scoreborad entry/struct (the members of the structs themselves
will still be aligned according to their type, so they can also be
safely accessed).


Wasn't the bus error occuring in

ws->last_used = apr_time_now();

and the address is

(dbx) print &(ws->last_used)
>last_used = 0x7bb00094

not divisible by 8 although the data type (not pointer size) is 64 Bit.

Regards,

Rainer

Re: 2.4.17 alignment issue sparc/ia64

[Jim Jagielski]

> 
> Yes but ws itself isn't aligned either:
>(dbx) print ws
>ws = 0x7bb00044
> which is IMHO the issue.
> 
> Align ws and everything goes well (at least I think :p ).
> 

It better! :)

Re: 2.4.17 alignment issue sparc/ia64

[Eric Covener]

On Fri, Oct 16, 2015 at 4:02 AM, Yann Ylavic  wrote:
> We should do something like the following patch:

Promising so far with my two one-off testcases, putting it through a
longer test now.

Thanks!

Re: 2.4.17 alignment issue sparc/ia64

[Yann Ylavic]

On Fri, Oct 16, 2015 at 4:16 PM, Eric Covener  wrote:
> On Fri, Oct 16, 2015 at 4:02 AM, Yann Ylavic  wrote:
>> We should do something like the following patch:
>
> Promising so far with my two one-off testcases, putting it through a
> longer test now.

Committed in r1709008 (was afraid the discussion could have led to the
wrong one being tested...).
Will revert if it fails.

2.4.17 alignment issue sparc/ia64

[Eric Covener]

We recently merged 2.4.17 and saw some bus errors on hp/ia64 and
solaris/sparc64. Selectively backing things out, it appears that the
SO_REUSEPORT patch causes the worker_score to no longer (necessarily)
be double-word aligned.

I'm able to hit the same thing with the httpd-2.4.x branch.  I have a
particular config that I can start with:

/bin/apachectl  -f conf/testcase.conf -k start -c "ServerLimit 1" -DNO_DETACH
or
/bin/apachectl  -f conf/testcase.conf -k start -c "ServerLimit 2" -DNO_DETACH

And the former will get a bus error and the latter will not.

(stuff from my draft email below is not vanilla httpd-2.4.x)

t@1 (l@1) program terminated by signal BUS (Bus Error)
Current function is update_child_status_internal
  479   ws->last_used = apr_time_now();
(dbx) where
current thread: t@1
=>[1] update_child_status_internal(child_num = 0, thread_num = 0,
status = 0, c = (nil), r = (nil)), line 479 in "scoreboard.c"
  [2] ap_update_child_status_from_indexes(child_num = 0, thread_num =
0, status = 0, r = (nil)), line 529 in "scoreboard.c"
  [3] server_main_loop(remaining_children_to_start = 0, num_buckets =
1), line 1879 in "worker.c"
  [4] worker_run(_pconf = 0x100205f68, plog = 0x100234158, s =
0x10020e340), line 2056 in "worker.c"
  [5] ap_run_mpm(0x100205f68, 0x100234158, 0x10020e340, 0x5, 0x0,
0x0), at 0x10005a8a4
  [6] main(argc = 7, argv = 0x77f8), line 878 in "main.c"
(dbx)
(dbx) print ws
ws = 0x7bb00044
(dbx) print &(ws->last_used)
>last_used = 0x7bb00094

(both of these are unaligned, when I log their values w/o the
SO_REUSEPORT they're always aligned)

Experimentation is SLOW on my sparc system, so I thought I'd throw up
a flare for help sooner rather than later in case anyone has a clue.
I hope to be able to try to back out some more things on the vanilla
httpd-2.4.x

-- 
Eric Covener
cove...@gmail.com