Page MenuHomeFreeBSD

Implement atomic_swap_xxx() for all platforms
ClosedPublic

Authored by hselasky on Dec 6 2018, 2:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 12:43 PM
Unknown Object (File)
Fri, Dec 20, 6:14 AM
Unknown Object (File)
Tue, Dec 10, 7:53 PM
Unknown Object (File)
Mon, Nov 25, 12:18 AM
Unknown Object (File)
Nov 23 2024, 6:07 AM
Unknown Object (File)
Nov 23 2024, 5:27 AM
Unknown Object (File)
Oct 28 2024, 2:35 AM
Unknown Object (File)
Oct 23 2024, 11:11 PM

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 21392

Event Timeline

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.Dec 6 2018, 2:56 PM
sys/mips/include/atomic.h
794

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

I suspect this might not compile.

hselasky added inline comments.
sys/powerpc/include/atomic.h
856

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

hselasky added inline comments.
sys/mips/include/atomic.h
794

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

sys/mips/include/atomic.h
794

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

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 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

sys/arm/include/atomic-v6.h
776

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

sys/mips/include/atomic.h
761

Style does not allow to initialize at declaration.

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.Dec 9 2018, 5:11 PM

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.Dec 10 2018, 9:20 AM
sys/powerpc/include/atomic.h
856

But you still use u_int there ?

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.Dec 10 2018, 1:38 PM
This revision was automatically updated to reflect the committed changes.
arichardson added inline comments.
head/sys/mips/include/atomic.h
803 ↗(On Diff #51810)

This seems wrong: long is 64 bytes on MIPS64.

hselasky added inline comments.
head/sys/mips/include/atomic.h
803 ↗(On Diff #51810)

I'll fix.

head/sys/mips/include/atomic.h
782 ↗(On Diff #51810)

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

hselasky added inline comments.
head/sys/mips/include/atomic.h
782 ↗(On Diff #51810)

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

head/sys/mips/include/atomic.h
782 ↗(On Diff #51810)

I can fix it...