Page MenuHomeFreeBSD
Differential D18450

Implement atomic_swap_xxx() for all platforms
ClosedPublic
Actions

Authored by • hselasky on Dec 6 2018, 2:55 PM.
Tags
None
Referenced Files
F105757709: D18450.diff
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)
Sat, Nov 23, 6:07 AM
Unknown Object (File)
Sat, Nov 23, 5:27 AM
Unknown Object (File)
Oct 28 2024, 2:35 AM
Unknown Object (File)
Oct 23 2024, 11:11 PM
Unknown Object (File)
Oct 2 2024, 6:17 AM
View All 100 Files
Subscribers
andrew
arichardson
emaste
imp
jhibbits

Details

Reviewers
kib
Commits
rS341789: Remove no longer needed ifdefs in the LinuxKPI, after r341787.
rS341787: Implement atomic_swap_xxx() for all platforms.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

• hselasky created this revision.Dec 6 2018, 2:55 PM
Herald added subscribers: emaste, jhibbits, andrew, imp. · View Herald TranscriptDec 6 2018, 2:55 PM
Harbormaster completed remote builds in B21392: Diff 51651.Dec 6 2018, 2:55 PM
• hselasky added a comment.Dec 6 2018, 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.Dec 6 2018, 2:56 PM
kib added inline comments.Dec 6 2018, 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.Dec 6 2018, 4:05 PM

Compile fix.

Harbormaster completed remote builds in B21395: Diff 51656.Dec 6 2018, 4:05 PM
• hselasky marked an inline comment as done.Dec 6 2018, 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.Dec 6 2018, 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.Dec 6 2018, 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.Dec 6 2018, 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

Harbormaster completed remote builds in B21408: Diff 51684.Dec 6 2018, 9:26 PM
kib added inline comments.Dec 6 2018, 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.Dec 9 2018, 4:47 PM

Compile fixes.

Harbormaster completed remote builds in B21454: Diff 51777.Dec 9 2018, 4:49 PM
kib accepted this revision.Dec 9 2018, 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.Dec 9 2018, 5:11 PM
• hselasky updated this revision to Diff 51801.Dec 10 2018, 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.Dec 10 2018, 9:20 AM
Harbormaster completed remote builds in B21473: Diff 51801.Dec 10 2018, 9:21 AM
kib added inline comments.Dec 10 2018, 9:40 AM
sys/powerpc/include/atomic.h
856 ↗(On Diff #51801)

But you still use u_int there ?

• hselasky added a comment.EditedDec 10 2018, 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.Dec 10 2018, 1:38 PM
Closed by commit rS341787: Implement atomic_swap_xxx() for all platforms. (authored by • hselasky). · Explain Why
This revision was automatically updated to reflect the committed changes.
arichardson added a subscriber: arichardson.Dec 10 2018, 4:14 PM
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.Dec 10 2018, 4:40 PM
• hselasky added inline comments.
head/sys/mips/include/atomic.h
803

I'll fix.

imp added inline comments.Dec 10 2018, 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.Dec 10 2018, 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.Dec 10 2018, 9:39 PM
head/sys/mips/include/atomic.h
782

I can fix it...

Revision Contents
Changeset List

PathSize
head/
sys/
arm/
include/
7 lines
mips/
include/
64 lines
powerpc/
include/
3 lines

Diff 51810

View Options

head/sys/arm/include/atomic.h

Loading...
View Options

head/sys/mips/include/atomic.h

Loading...
View Options

head/sys/powerpc/include/atomic.h

Loading...