Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Dinar Temirbulatov-2
Hi,
Another version of change, I added
atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_rel
and atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_rel
defenitions and also for gcc-4.7 and higher in the case of unsupported
atomic compare and swap operation, it uses the kernel helper inlines.
Tested on arm a9 with no new regressions. Ok to commit?
                    thanks, Dinar.
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Dinar Temirbulatov-2
Hi,
Another version of change, I added
atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_rel
and atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_rel
defenitions and also for gcc-4.7 and higher in the case of unsupported
atomic compare and swap operation, it uses the kernel helper inlines.
Tested on arm a9 with no new regressions. Ok to commit?
Oh, sorry. I missed to attach the change. Here it is.
                    thanks, Dinar.

arm_atomic.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Joseph Myers
On Mon, 2 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Another version of change, I added
> atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_rel
> and atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_rel
> defenitions and also for gcc-4.7 and higher in the case of unsupported
> atomic compare and swap operation, it uses the kernel helper inlines.
> Tested on arm a9 with no new regressions. Ok to commit?
> Oh, sorry. I missed to attach the change. Here it is.

For subsequent patch revisions, please note there should be an extra space
after "#" for preprocessor directives inside #if conditionals, one per
level of #if nesting (other than toplevel multiple-inclusion guards) -
which means that if conditioning existing code, you need to adjust
directives inside that code.

This patch appears to have too much duplication.  For example, you
duplicate the definition of __arm_assisted_compare_and_exchange_val_32_acq
- but that should not need any extra conditionals at all (beyond the
existing #ifndef __arm_assisted_compare_and_exchange_val_32_acq), there's
no reason ever not to define it.  Similarly, you duplicate
__arch_compare_and_exchange_val_64_acq, but with proper #if structure
there should only need to be one copy of the version that uses
__arm_link_error.

What I think you should aim for is that each definition, or small group of
definitions, uses conditionals in the form

#if __GNUC_PREREQ (4, 7) && defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
/* Version using __atomic_*.  */
#elif defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
/* Version using __sync_*.  */
#else
/* Version using __arm_assisted_*.  */
#endif

with cases omitted if not useful for that particular macro (this may
include some macros not being defined at all in some cases).  So don't
insert any global conditionals affecting all the existing definitions at
all - look at each block of conditionals and add a third case as needed,
along with any new macros (again with conditionals in that form) that are
appropriate.

Where you use abort () in some definitions, use __arm_link_error ()
instead, like for the existing definitions.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

RE: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Abhishek Deb
From the functionality point of view, the changes look ok.

An objdump of the atomic_compare_and_exchange_bool_acq shows the following:

  60:   e1903f9f        ldrex   r3, [r0]
  64:   e1530002        cmp     r3, r2
  68:   1a000003        bne     7c <local__lll_lock_wait+0x34>
  6c:   e1804f91        strex   r4, r1, [r0]
  70:   e3540000        cmp     r4, #0
  74:   1afffff9        bne     60 <local__lll_lock_wait+0x18>
  78:   f57ff05f        dmb     sy

As you can see it contains only one dmb in the end and not in the beginning, which is in-line with acquire semantics to not allow any code hoisting.

Abhishek

-----Original Message-----
From: Joseph Myers [mailto:[hidden email]]
Sent: Monday, September 02, 2013 8:08 AM
To: Dinar Temirbulatov
Cc: [hidden email]; Abhishek Deb
Subject: Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

On Mon, 2 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Another version of change, I added
> atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_re
> l and
> atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_
> rel defenitions and also for gcc-4.7 and higher in the case of
> unsupported atomic compare and swap operation, it uses the kernel
> helper inlines.
> Tested on arm a9 with no new regressions. Ok to commit?
> Oh, sorry. I missed to attach the change. Here it is.

For subsequent patch revisions, please note there should be an extra space after "#" for preprocessor directives inside #if conditionals, one per level of #if nesting (other than toplevel multiple-inclusion guards) - which means that if conditioning existing code, you need to adjust directives inside that code.

This patch appears to have too much duplication.  For example, you duplicate the definition of __arm_assisted_compare_and_exchange_val_32_acq
- but that should not need any extra conditionals at all (beyond the existing #ifndef __arm_assisted_compare_and_exchange_val_32_acq), there's no reason ever not to define it.  Similarly, you duplicate __arch_compare_and_exchange_val_64_acq, but with proper #if structure there should only need to be one copy of the version that uses __arm_link_error.

What I think you should aim for is that each definition, or small group of definitions, uses conditionals in the form

#if __GNUC_PREREQ (4, 7) && defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
/* Version using __atomic_*.  */
#elif defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
/* Version using __sync_*.  */
#else
/* Version using __arm_assisted_*.  */
#endif

with cases omitted if not useful for that particular macro (this may include some macros not being defined at all in some cases).  So don't insert any global conditionals affecting all the existing definitions at all - look at each block of conditionals and add a third case as needed, along with any new macros (again with conditionals in that form) that are appropriate.

Where you use abort () in some definitions, use __arm_link_error () instead, like for the existing definitions.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Dinar Temirbulatov-2
In reply to this post by Joseph Myers
Hi,
Here is updated version of change. Ok to commit?
                thanks, Dinar.

On Mon, Sep 2, 2013 at 7:08 PM, Joseph S. Myers <[hidden email]> wrote:

> On Mon, 2 Sep 2013, Dinar Temirbulatov wrote:
>
>> Hi,
>> Another version of change, I added
>> atomic_compare_and_exchange_val_acq/atomic_compare_and_exchange_val_rel
>> and atomic_compare_and_exchange_bool_acq/atomic_compare_and_exchange_boot_rel
>> defenitions and also for gcc-4.7 and higher in the case of unsupported
>> atomic compare and swap operation, it uses the kernel helper inlines.
>> Tested on arm a9 with no new regressions. Ok to commit?
>> Oh, sorry. I missed to attach the change. Here it is.
>
> For subsequent patch revisions, please note there should be an extra space
> after "#" for preprocessor directives inside #if conditionals, one per
> level of #if nesting (other than toplevel multiple-inclusion guards) -
> which means that if conditioning existing code, you need to adjust
> directives inside that code.
>
> This patch appears to have too much duplication.  For example, you
> duplicate the definition of __arm_assisted_compare_and_exchange_val_32_acq
> - but that should not need any extra conditionals at all (beyond the
> existing #ifndef __arm_assisted_compare_and_exchange_val_32_acq), there's
> no reason ever not to define it.  Similarly, you duplicate
> __arch_compare_and_exchange_val_64_acq, but with proper #if structure
> there should only need to be one copy of the version that uses
> __arm_link_error.
>
> What I think you should aim for is that each definition, or small group of
> definitions, uses conditionals in the form
>
> #if __GNUC_PREREQ (4, 7) && defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
> /* Version using __atomic_*.  */
> #elif defined __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
> /* Version using __sync_*.  */
> #else
> /* Version using __arm_assisted_*.  */
> #endif
>
> with cases omitted if not useful for that particular macro (this may
> include some macros not being defined at all in some cases).  So don't
> insert any global conditionals affecting all the existing definitions at
> all - look at each block of conditionals and add a third case as needed,
> along with any new macros (again with conditionals in that form) that are
> appropriate.
>
> Where you use abort () in some definitions, use __arm_link_error ()
> instead, like for the existing definitions.
>
> --
> Joseph S. Myers
> [hidden email]

arm_atomic7.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Joseph Myers
On Thu, 5 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Here is updated version of change. Ok to commit?

You appear to insert a '$' after '\' on one line - that can't be right.  
Please try to test all three cases in the code.

Where you have "typeof(*mem)", there should be a space before the '('.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

RE: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Abhishek Deb
Compilation with 4.8.0 succeeded, whereas compilation with 4.6.2 failed due to the trailing $ as pointed by Joseph.

Abhishek

-----Original Message-----
From: Joseph Myers [mailto:[hidden email]]
Sent: Thursday, September 05, 2013 8:48 AM
To: Dinar Temirbulatov
Cc: [hidden email]; Abhishek Deb; Maxim Kuvyrkov
Subject: Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

On Thu, 5 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Here is updated version of change. Ok to commit?

You appear to insert a '$' after '\' on one line - that can't be right.  
Please try to test all three cases in the code.

Where you have "typeof(*mem)", there should be a space before the '('.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Dinar Temirbulatov-2
Hi,
Another updated version of the change with just eliminated $. Tested
glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only)
with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3)
arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
with no new regressions. Ok to commit?
                          thanks, Dinar.
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Dinar Temirbulatov-2
In reply to this post by Abhishek Deb
Hi,
Another updated version of the change with just eliminated $. Tested
glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only)
with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3)
arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
with no new regressions. Ok to commit?
                          thanks, Dinar.

arm_atomic8.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Abhishek Deb
Compilation OK and the generated code OK.

OK from my side.

Abhishek

-----Original Message-----
From: Dinar Temirbulatov [mailto:[hidden email]]
Sent: Wednesday, September 11, 2013 12:28 PM
To: Abhishek Deb
Cc: Joseph Myers; [hidden email]; Maxim Kuvyrkov
Subject: Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Hi,
Another updated version of the change with just eliminated $. Tested glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only) with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3) arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
with no new regressions. Ok to commit?
                          thanks, Dinar.
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Joseph Myers
In reply to this post by Dinar Temirbulatov-2
On Wed, 11 Sep 2013, Dinar Temirbulatov wrote:

> Hi,
> Another updated version of the change with just eliminated $. Tested
> glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only)
> with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3)
> arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
> with no new regressions. Ok to commit?

This version is OK.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Dinar Temirbulatov-2
Hello Joseph,
Oh, I don't have any write permissions to the project.
               thanks, Dinar.

On Fri, Sep 13, 2013 at 9:14 PM, Joseph S. Myers
<[hidden email]> wrote:

> On Wed, 11 Sep 2013, Dinar Temirbulatov wrote:
>
>> Hi,
>> Another updated version of the change with just eliminated $. Tested
>> glibc internal testsuite with: 1) arm v4t gcc-4.5.3 with
>> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 undefined (kernel helpers only)
>> with no new regrssions, 2) arm with FSF gcc-4.6.4 on  armv7-a with
>> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 defined with no new regrssions, 3)
>> arm FSF gcc-4.7.3 on armv7-a with __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
>> with no new regressions. Ok to commit?
>
> This version is OK.
>
> --
> Joseph S. Myers
> [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Joseph Myers
On Sat, 14 Sep 2013, Dinar Temirbulatov wrote:

> Hello Joseph,
> Oh, I don't have any write permissions to the project.

Please provide a GNU-format ChangeLog entry (as detailed in the
contribution checklist) for the patch (probably using [BZ #15640], as I
understand it).

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Maxim Kuvyrkov-3
On 19/09/2013, at 5:16 AM, Joseph S. Myers <[hidden email]> wrote:

> On Sat, 14 Sep 2013, Dinar Temirbulatov wrote:
>
>> Hello Joseph,
>> Oh, I don't have any write permissions to the project.
>
> Please provide a GNU-format ChangeLog entry (as detailed in the
> contribution checklist) for the patch (probably using [BZ #15640], as I
> understand it).

I've checked in Dinar's patch with appropriate ChangeLog and closed the bug.

Thanks,

--
Maxim Kuvyrkov
www.kugelworks.com

Reply | Threaded
Open this post in threaded view
|

Re: [Patch] ARM define atomic_exchange_acq/atomic_exchange_rel to __atomic_exchange_n

Joseph Myers
Note for future reference to use git commit --author= when committing
patches for someone else.

--
Joseph S. Myers
[hidden email]