Page MenuHomeFreeBSD

powerpc: implement atomic_set/clear_16
ClosedPublic

Authored by kib on Fri, Sep 19, 6:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 2, 4:40 PM
Unknown Object (File)
Wed, Oct 1, 5:12 PM
Unknown Object (File)
Sun, Sep 28, 6:29 AM
Unknown Object (File)
Thu, Sep 25, 4:28 PM
Unknown Object (File)
Sun, Sep 21, 10:33 PM
Unknown Object (File)
Sun, Sep 21, 8:56 AM
Unknown Object (File)
Sat, Sep 20, 11:03 PM
Unknown Object (File)
Sat, Sep 20, 10:14 PM
Subscribers

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib requested review of this revision.Fri, Sep 19, 6:03 PM
kib added a reviewer: jhibbits.
kib removed a subscriber: jhibbits.

Did not even compiled yet.

Looks like we need this on riscv as well

Move to _atomic_subword.h

jrtc27 added inline comments.
sys/sys/_atomic_subword.h
209

Would the normal pattern not be:

#ifndef atomic_set_16
static __inline void
atomic_set_short(volatile uint16_t *p, uint16_t bit)
{
    ...
}
#endif

and then let each arch do #define atomic_set_short atomic_set_16? That is, (a) invert the direction for the #define'ing (b) split the #define out to per-arch headers (c) guard each definition.

This both deviates from the current style but also won't honour architectures that already provide this width of atomic.

The implementation seems ok to me.

sys/sys/_atomic_subword.h
209

Indeed, why not follow the existing pattern?

This revision is now accepted and ready to land.Fri, Sep 19, 7:55 PM

Add arm.
Provide atomic_set/clear_16 in subword_atomic.h.

This passed tinderbox.

This revision now requires review to proceed.Sat, Sep 20, 2:56 AM
markj added inline comments.
sys/sys/_atomic_subword.h
210
212
214
224

Same in this function.

This revision is now accepted and ready to land.Sat, Sep 20, 12:18 PM
kib marked 6 inline comments as done.

Both args' types of atomic_set/clear_16 from _atomic_subword.h were changed to uint16_t.

This revision now requires review to proceed.Sat, Sep 20, 6:45 PM
sys/arm/include/atomic.h
1105–1106

Comment is now stale (I *think* prior to this change it was accurate?)

sys/powerpc/include/atomic.h
1135

Should this and the two new #defines not just be hoisted? AFAIUI the only reason the header is made conditional today is because it's unnecessary, not because there's a problem with making it unconditional?

sys/arm/include/atomic.h
1105–1106

Why? I did not changed anything WRT atomic_load. Do you mean that atomic_clear/set_16 should be included in the list? [I would rather remove this comment at all then]

sys/powerpc/include/atomic.h
1135

Sorry I do not understand your comment.

The subword use is needed for !206_ATOMIC case, but is too much for 206 so I added the functions definitions directly.

sys/arm/include/atomic.h
1105–1106

Yes, I mean that the list is now misleading since the header is no longer included just for the functions enumerated here. Deleting it is also fine with me, neither of the other arches here include such a list.

sys/powerpc/include/atomic.h
1135

I mean that, since the #else is a copy of the subword code, ideally we'd just include the subword header, in such a way that it doesn't pick up any of the other subword bits, which I suspect would already be the case. But if you would rather not do that still I won't object further.

kib marked 2 inline comments as done.Sat, Sep 20, 7:11 PM
kib added inline comments.
sys/powerpc/include/atomic.h
1135

I do not want to spend that much efforts on ppc, esp. because I cannot test (but not only).

kib marked an inline comment as done.

Remove stale arm comment (which might be wrong even before the patch)

sys/powerpc/include/atomic.h
1135

Ok fair enough

This revision is now accepted and ready to land.Sat, Sep 20, 7:18 PM