Differential D20204

First cut at cleaning up MP_RING_NO_64BIT_ATOMICS code
AbandonedPublic
Actions

Authored by shurd on May 8 2019, 10:33 PM.

Details

Reviewers

None

Group Reviewers

iflib

Summary

Clean up code used when MP_RING_NO_64BIT_ATOMICS is defined.
ie: defined(powerpc) || defined(mips) || defined(i386)

Highlights:
Unlock ring during drain() callback to allow filling while draining
Grab ring mutex (if present) before reading ring state
Remove dead code from enqueue

Test Plan

I'm going to build amd64 with MP_RING_NO_64BIT_ATOMICS and
try it out, but it would be nice to have a native platform tested.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 24163
Build 23010: arc lint + arc unit

Event Timeline

shurd created this revision.May 8 2019, 10:33 PM

Herald added a reviewer: iflib. · View Herald TranscriptMay 8 2019, 10:33 PM

Herald added subscribers: ae, imp. · View Herald Transcript

Harbormaster completed remote builds in B24163: Diff 57200.May 8 2019, 10:33 PM

marius added a subscriber: marius.May 8 2019, 11:39 PM

marius added inline comments.

sys/net/mp_ring.c
103	I'd suggest to move this to mp_ring.h as a static inline (maybe even static __always_inline) function to increase the likelihood of the compiler actually inlining the thing.
sys/net/mp_ring.h
51	Hrm, I think something like the following would be somewhat clearer and more readable: #ifndef MP_RING_NO_64BIT_ATOMICS typedef uint64_t state_t #else typedef volatile uint64_t state_t #endif struct ifmp_ring { state_t state __aligned(CACHE_LINE_SIZE);

marius added inline comments.May 8 2019, 11:54 PM

sys/net/mp_ring.c
103	On a closer look, I don't think it actually makes sense to grab the lock for reading the ring state in the places ifmp_ring_get_state() currently is used.