Page MenuHomeFreeBSD

First cut at cleaning up MP_RING_NO_64BIT_ATOMICS code
Needs ReviewPublic

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
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 24163
Build 23010: arc lint + arc unit

Event Timeline

shurd created this revision.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.