Page MenuHomeFreeBSD

Implement atomic_swap_xxx() for all platforms
ClosedPublic

Authored by hselasky on Thu, Dec 6, 2:55 PM.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

hselasky created this revision.Thu, Dec 6, 2:55 PM

Currently running a universe build.

hselasky retitled this revision from Implement atomic_swap_long() for all platforms to Implement atomic_swap_long() and atomic_swap_int() for all platforms.Thu, Dec 6, 2:56 PM
kib added inline comments.Thu, Dec 6, 3:16 PM
sys/mips/include/atomic.h
794 ↗(On Diff #51651)

Do we provide ack/rel variants of swap on any other arch ?
Also, if you use fcmpset_acq/rel, I do not see why do you need additional thread fences.

sys/powerpc/include/atomic.h
856 ↗(On Diff #51651)

I suspect this might not compile.

hselasky updated this revision to Diff 51656.Thu, Dec 6, 4:05 PM

Compile fix.

hselasky marked an inline comment as done.Thu, Dec 6, 4:06 PM
hselasky added inline comments.
sys/powerpc/include/atomic.h
856 ↗(On Diff #51651)

Other places I see the same #define . I'm currently doing a universe, so I will know.

hselasky marked an inline comment as done.Thu, Dec 6, 4:07 PM
hselasky added inline comments.
sys/mips/include/atomic.h
794 ↗(On Diff #51651)

No, usually not. Do you want me to clear those variants?

kib added inline comments.Thu, Dec 6, 4:18 PM
sys/mips/include/atomic.h
794 ↗(On Diff #51651)

Yes, I think it is better not to do that, both due to the precedent and because the implementation is somewhat questionable.

sys/powerpc/include/atomic.h
856 ↗(On Diff #51651)

It would work if the real implementation function does explicit cast to uint32_t or uint32_t is typedef for long. If it is typedef to int, then this should break.

hselasky updated this revision to Diff 51684.Thu, Dec 6, 9:26 PM
hselasky retitled this revision from Implement atomic_swap_long() and atomic_swap_int() for all platforms to Implement atomic_swap_xxx() for all platforms.

Update as per suggestion from @kib

kib added inline comments.Thu, Dec 6, 10:01 PM
sys/arm/include/atomic-v6.h
776 ↗(On Diff #51684)

I think you can put this function into arm/include/atomic.h and avoid writing it twice.

sys/mips/include/atomic.h
761 ↗(On Diff #51684)

Style does not allow to initialize at declaration.

hselasky updated this revision to Diff 51777.Sun, Dec 9, 4:47 PM

Compile fixes.

kib accepted this revision.Sun, Dec 9, 5:11 PM

Asm versions of the mips swap() primitives would be less ugly, but I think this can be taken care of by the mips people.

This revision is now accepted and ready to land.Sun, Dec 9, 5:11 PM
hselasky updated this revision to Diff 51801.Mon, Dec 10, 9:20 AM

More minor build fixes. u_int is not always defined when including atomic.h. Use uint32_t instead.

This revision now requires review to proceed.Mon, Dec 10, 9:20 AM
kib added inline comments.Mon, Dec 10, 9:40 AM
sys/powerpc/include/atomic.h
856 ↗(On Diff #51801)

But you still use u_int there ?

hselasky added a comment.EditedMon, Dec 10, 10:57 AM

Yes, but those are macros, so they are not compiled when the header file is included.

I'm seeing something like this:

so9899:1999 -Werror  /usr/img/freebsd/sys/dev/cfe/cfe_api.c
In file included from /usr/img/freebsd/sys/sys/systm.h:44,
                 from /usr/img/freebsd/sys/dev/cfe/cfe_api.h:67,
                 from /usr/img/freebsd/sys/dev/cfe/cfe_api.c:54:
./machine/atomic.h: In function 'atomic_swap_long':
./machine/atomic.h:803: error: expected ')' before 'u_int'
cc1: warnings being treated as errors
./machine/atomic.h:803: warning: type defaults to 'int' in declaration of 'type name'
./machine/atomic.h:803: error: 'u_int' undeclared (first use in this function)
./machine/atomic.h:803: error: (Each undeclared identifier is reported only once
./machine/atomic.h:803: error: for each function it appears in.)
./machine/atomic.h:803: error: expected expression before ')' token
./machine/atomic.h:803: warning: passing argument 1 of 'atomic_fcmpset_32' makes pointer from integer without a cast
./machine/atomic.h: In function 'atomic_swap_ptr':
./machine/atomic.h:815: error: expected ')' before 'u_int'
./machine/atomic.h:815: warning: type defaults to 'int' in declaration of 'type name'
./machine/atomic.h:815: error: 'u_int' undeclared (first use in this function)
./machine/atomic.h:815: error: expected expression before ')' token
./machine/atomic.h:815: warning: passing argument 1 of 'atomic_fcmpset_32' makes pointer from integer without a cast
--- bcm_machdep.o ---
This revision was not accepted when it landed; it landed in state Needs Review.Mon, Dec 10, 1:38 PM
Closed by commit rS341787: Implement atomic_swap_xxx() for all platforms. (authored by hselasky, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
arichardson added inline comments.
head/sys/mips/include/atomic.h
803

This seems wrong: long is 64 bytes on MIPS64.

hselasky marked an inline comment as done.Mon, Dec 10, 4:40 PM
hselasky added inline comments.
head/sys/mips/include/atomic.h
803

I'll fix.

imp added inline comments.Mon, Dec 10, 6:10 PM
head/sys/mips/include/atomic.h
782

This still isn't defined on 32-bit mips then, is it?

hselasky marked an inline comment as done.Mon, Dec 10, 9:37 PM
hselasky added inline comments.
head/sys/mips/include/atomic.h
782

I guess someone with MIPS platform knowledge will need to fix this.

imp added inline comments.Mon, Dec 10, 9:39 PM
head/sys/mips/include/atomic.h
782

I can fix it...