Hello!

On porting php-fpm to ARM architecture (the SheevaPlug, to be more
specific) I've noticed in sapi/cgi/fpm/fpm_atomic.h you define your
own "atomic_cmp_set" functions for the spinlock. Please forgive me the
question, I am no C expert, but why not using GCC's
__sync_bool_compare_and_swap?

--
W-Mark Kubacki

On Wed, Sep 23, 2009 at 4:59 PM, W-Mark Kubacki
<wmark.google@hurrikane.de> wrote:
>
> Hello!
>
> On porting php-fpm to ARM architecture (the SheevaPlug, to be more
> specific) I've noticed in sapi/cgi/fpm/fpm_atomic.h you define your
> own "atomic_cmp_set" functions for the spinlock. Please forgive me the
> question, I am no C expert, but why not using GCC's
> __sync_bool_compare_and_swap?
>
> --
> W-Mark Kubacki
>

Not sure as this is a very low-level implementation question. Andrei
might know, but it's difficult to reach him. Make sure to ask on the
Russian list 'highload-php-ru'


Best regards,

dreamcat4
dreamcat4@gmail.com

On 23 Sep., 19:12, dreamcat four <dreamc...@gmail.com> wrote:
> On Wed, Sep 23, 2009 at 4:59 PM, W-Mark Kubacki
>
> <wmark.goo...@hurrikane.de> wrote:
>
> > Hello!
>
> > On porting php-fpm to ARM architecture (the SheevaPlug, to be more
> > specific) I've noticed in sapi/cgi/fpm/fpm_atomic.h you define your
> > own "atomic_cmp_set" functions for the spinlock. Please forgive me the
> > question, I am no C expert, but why not using GCC's
> > __sync_bool_compare_and_swap?
>
> Not sure as this is a very low-level implementation question. Andrei
> might know, but it's difficult to reach him. Make sure to ask on the
> Russian list 'highload-php-ru'

Well, I don't speak Russian.

Nevertheless, in the meanwhile I managed to compile php-fpm on ARM/
SheevaPlug and it's running fine.
You can find the patch to php-fpm (to be applied afterwards) here:
https://svn.hurrikane.de/all/ossdl/dev-lang/php/files/php-5.2.10-fpm-0.5.13.diff-gcc.patch

The binary can be obtained from:
http://binhost.ossdl.de/armv5tel-softfloat-linux-gnueabi/dev-lang/php-5.2.10_p1-r1.tbz
(Gentoo, GCC 4.4.1)

--
Mark

On 24 Sep., 00:44, W-Mark Kubacki <wmark.goo...@hurrikane.de> wrote:
>[...]
>
> The binary can be obtained from:http://binhost.ossdl.de/armv5tel-softfloat-linux-gnueabi/dev-lang/php...
> (Gentoo, GCC 4.4.1)

Sorry, forgot to copy the trailing character. Here's the correct link:
http://binhost.ossdl.de/armv5tel-softfloat-linux-gnueabi/dev-lang/php-5.2.10_p1-r1.tbz2

--
Mark

Mark,
I'm sorry but we cannot remove the following line:
#error unsupported processor. please write a patch and send it to me

Also, why did you only define the uints and not the ints?
Doesn't arm have an int type?

For a new additional architecture, why not:

#elif ( __myarch__ || __myarch )

As per the existing two architectures ?
Then can nest the #if arch64 inside that if necessary

For example:

#elif ( __myarch__ || __myarch )
#if (__arch64__ || __arch64)
/* define 64bit ints */
#else
/* define 32bit ints */
#fi
#define atomic_cmp_set(a,b,c) __sync_bool_compare_and_swap(a,b,c)
#else

#error unsupported processor. please write a patch and send it to me

#endif



Best regards,

dreamcat4
dreamcat4@gmail.com

On Wed, Sep 23, 2009 at 11:46 PM, W-Mark Kubacki
<wmark.google@hurrikane.de> wrote:
>
> On 24 Sep., 00:44, W-Mark Kubacki <wmark.goo...@hurrikane.de> wrote:
>>[...]
>>
>> The binary can be obtained from:http://binhost.ossdl.de/armv5tel-softfloat-linux-gnueabi/dev-lang/php...
>> (Gentoo, GCC 4.4.1)
>
> Sorry, forgot to copy the trailing character. Here's the correct link:
> http://binhost.ossdl.de/armv5tel-softfloat-linux-gnueabi/dev-lang/php-5.2.10_p1-r1.tbz2
>
> --
> Mark

On 24 Sep., 10:02, dreamcat four <dreamc...@gmail.com> wrote:
> Mark,
> I'm sorry but we cannot remove the following line:
> #error unsupported processor. please write a patch and send it to me
>
> Also, why did you only define the uints and not the ints?
> Doesn't arm have an int type?
>
> For a new additional architecture, why not:
>
> #elif ( __myarch__ || __myarch )
>
> As per the existing two architectures ?

You're right. I could add another #elif, but I feel the code will be
much more portable if the #if (__myarch__) sections got deleted in
favour of __sync_bool_compare_and_swap - which works on x86 and amd64
as well. (Which might be a coincidence, though.)
Then, the only left #if/else/endif section would be for checking
whether the compiler is GCC at all.

I've not seen the ints being used, therefore I've skipped them for
brevity as Marcin Ochab did in his Sparc patch. Of course ARM has that
data type.

> Then can nest the #if arch64 inside that if necessary
> [...]

If we used the portable code, we should use the pre-defined data types
- and there will be no longer an #if arch64 switch. It has already
been done in libc (or whatever library you feel suits best).

But, I am no C expert so I might be completely wrong. It just appears
to equivalent to me (as in blackbox unit tests) and resulted in same
php-fpm's runtime behaviour.

--
Mark

On Thu, Sep 24, 2009 at 9:59 AM, W-Mark Kubacki
<wmark.google@hurrikane.de> wrote:
>
> On 24 Sep., 10:02, dreamcat four <dreamc...@gmail.com> wrote:
>> Mark,
>> I'm sorry but we cannot remove the following line:
>> #error unsupported processor. please write a patch and send it to me
>>
>> Also, why did you only define the uints and not the ints?
>> Doesn't arm have an int type?
>>
>> For a new additional architecture, why not:
>>
>> #elif ( __myarch__ || __myarch )
>>
>> As per the existing two architectures ?
>
> You're right. I could add another #elif, but I feel the code will be
> much more portable if the #if (__myarch__) sections got deleted in
> favour of __sync_bool_compare_and_swap - which works on x86 and amd64
> as well. (Which might be a coincidence, though.)
> Then, the only left #if/else/endif section would be for checking
> whether the compiler is GCC at all.

Yeah, we just don't want to run any risks on the stable branch as so
many people rely upon it. (and currently there's no unstable branch.

So in short we would prefer just to add what's needed on a
case-by-case basis, with a view to re-factoring the code later (in an
unstable development branch).

> I've not seen the ints being used, therefore I've skipped them for
> brevity as Marcin Ochab did in his Sparc patch. Of course ARM has that

It seems we don't have Marcin's Sparc 64 patch yet either.
This one? http://forum.nginx.org/read.php?3,3533

>> Then can nest the #if arch64 inside that if necessary
>> [...]
>
> If we used the portable code, we should use the pre-defined data types
> - and there will be no longer an #if arch64 switch. It has already
> been done in libc (or whatever library you feel suits best).

We'd be happy to look at more patches here if you feel your original
patch using the arch64 declaration wouldn't work right. Do you which
is know the correct __arch__ declaration for your architecture?

Its unnecessary to remove / take out the existing i386/amd64 ASM. We
aren't sure if is better / worse than the gcc equivalent. Again,
because we only have a stable / production branch at this time. Of
course, we'd prefer to have less code and use the GCC function, if
possible. Except this thread says:

http://groups.google.com/group/cocotron-dev/browse_thread/thread/d51d2941e3aafd5d

that compare_swap doesn't universally work. Depending on which version
of GCC you have (>4.2) + and/or the cflags. Therefore you might see no
problem compiling on your machine, but others would experience
compilation errors and /or crashes. So this looks to be a common
practise for projects to re-write it.

Requires more expertise / knowledge. It's an open question whether we
would only need to enforce a check for the GCC version >= 4.2, so that
we can support ARM. Or would we need a cflags check also? This isn't
yet included in your patch. It's also worth pointing out that Marcin
didn't use the gcc function, but re-wrote it in SPARC 64 assembler. If
we had a similar one for ARM, we wouldn't require such configuration
checks.

So if anyone can help us solve these remaining issues, we would be
very grateful to include and support the ARM platform.


Best regards,

dreamcat4
dreamcat4@gmail.com

On Thu, Sep 24, 2009 at 11:31 AM, dreamcat four <dreamcat4@gmail.com> wrote:
> We'd be happy to look at more patches here if you feel your original
> patch using the arch64 declaration wouldn't work right. Do you which
> is know the correct __arch__ declaration for your architecture?

Still don't have here the conditional check. There could be may
combinations. So which are compatible for this GCC function ?

#if defined(__arm__)
#if defined(__arm__) && defined(__thumb__)
#if defined(__arm__) && !defined(__thumb__)
#if defined(__arm__) && defined(__ARM_ARCH_5TE__) && !defined(__thumb__)


And we might better support this if we can expect the people who
compile this, that they all use a GCC version greater than 4.1.


> Best regards,
>
> dreamcat4
> dreamcat4@gmail.com
>

On 24 Sep., 13:09, dreamcat four <dreamc...@gmail.com> wrote:
> On Thu, Sep 24, 2009 at 11:31 AM, dreamcat four <dreamc...@gmail.com> wrote:
> > We'd be happy to look at more patches here if you feel your original
> > patch using the arch64 declaration wouldn't work right. Do you which
> > is know the correct __arch__ declaration for your architecture?
>
> Still don't have here the conditional check. There could be may
> combinations. So which are compatible for this GCC function ?
>
> #if defined(__arm__)
> #if defined(__arm__) && defined(__thumb__)
> #if defined(__arm__) && !defined(__thumb__)
> #if defined(__arm__) && defined(__ARM_ARCH_5TE__) && !defined(__thumb__)

First and third match. (It's a "Feroceon 88FR131 rev 1 (v5l)")
I believe you should prefer the first, because ARMs could run "thumb"
code, too.

> And we might better support this if we can expect the people who
> compile this, that they all use a GCC version greater than 4.1.

If they don't, just emit that #error. If they do, maybe that GCC
intrinsic is (defined and) suitable enough (btw, on x86 it compiles to
cmpxchg[pl].).
But again, my question was an open one - thanks for the reading!
Did you compare arch/x86/include/asm/futex.h to arch/arm/include/asm/
futex.h? Maybe there is something you could adapt for php-fpm.

For x86 and amd64 it's okay to leave that Assembler code untouched: It
might happen that either GCC is not used at all and/or someone is
compiling PHP (and the patch) by Visual C++: After all, only the
target architectures of PHP are of relevance to php-fpm.

In my humble opinion, the implementation needing that spinlock should
be replaced. Maybe by a dedicated (pointer/event) queue or something
like that. But honestly speaking I have neither the knowledge nor the
time delving deeper into php-fpm.

--
Mark

On Thu, Sep 24, 2009 at 1:33 PM, W-Mark Kubacki
<wmark.google@hurrikane.de> wrote:
>
> On 24 Sep., 13:09, dreamcat four <dreamc...@gmail.com> wrote:
>> On Thu, Sep 24, 2009 at 11:31 AM, dreamcat four <dreamc...@gmail.com> wrote:
>> > We'd be happy to look at more patches here if you feel your original
>> > patch using the arch64 declaration wouldn't work right. Do you which
>> > is know the correct __arch__ declaration for your architecture?
>>
>> Still don't have here the conditional check. There could be may
>> combinations. So which are compatible for this GCC function ?
>>
>> #if defined(__arm__)
>> #if defined(__arm__) && defined(__thumb__)
>> #if defined(__arm__) && !defined(__thumb__)
>> #if defined(__arm__) && defined(__ARM_ARCH_5TE__) && !defined(__thumb__)
>
> First and third match. (It's a "Feroceon 88FR131 rev 1 (v5l)")
> I believe you should prefer the first, because ARMs could run "thumb"
> code, too.

Thank you for confirming this. We shall provide a simple __arm__
support under #ifdef

#if defined(__arm__)

Then someone else with ARM can improve later (like if they want thumb).

>> And we might better support this if we can expect the people who
>> compile this, that they all use a GCC version greater than 4.1.
>
> If they don't, just emit that #error. If they do, maybe that GCC
> intrinsic is (defined and) suitable enough (btw, on x86 it compiles to
> cmpxchg[pl].).
>    But again, my question was an open one - thanks for the reading!
> Did you compare arch/x86/include/asm/futex.h to arch/arm/include/asm/
> futex.h? Maybe there is something you could adapt for php-fpm.

we are also pushed for time here so I'll just try some GCC check and stop there.
Or it may be this configuration macro:
http://code.haskell.org/timber/rtsPOSIX/configure.ac

> For x86 and amd64 it's okay to leave that Assembler code untouched: It
> might happen that either GCC is not used at all and/or someone is
> compiling PHP (and the patch) by Visual C++: After all, only the
> target architectures of PHP are of relevance to php-fpm.
>
> In my humble opinion, the implementation needing that spinlock should
> be replaced. Maybe by a dedicated (pointer/event) queue or something
> like that. But honestly speaking I have neither the knowledge nor the
> time delving deeper into php-fpm.


We will also include the Marcin SPARC patch.

Best regards,

dreamcat4
dreamcat4@gmail.com