Hello Andrii,
This is quite exciting. Some comments below:
On Wed, May 27, 2020 at 11:24:08PM -0700, Andrii Nakryiko wrote:
[...]
> diff --git a/Documentation/litmus-tests/bpf-rb/bpf-rb+1p1c+bounded.litmus
> b/Documentation/litmus-tests/bpf-rb/bpf-rb+1p1c+bounded.litmus
> new file mode 100644
> index 000000000000..558f054fb0b4
> --- /dev/null
> +++ b/Documentation/litmus-tests/bpf-rb/bpf-rb+1p1c+bounded.litmus
> @@ -0,0 +1,91 @@
> +C bpf-rb+1p1c+bounded
> +
> +(*
> + * Result: Always
> + *
> + * This litmus test validates BPF ring buffer implementation under the
> + * following assumptions:
> + * - 1 producer;
> + * - 1 consumer;
> + * - ring buffer has capacity for only 1 record.
> + *
> + * Expectations:
> + * - 1 record pushed into ring buffer;
> + * - 0 or 1 element is consumed.
> + * - no failures.
> + *)
> +
> +{
> + atomic_t dropped;
> +}
> +
> +P0(int *lenFail, int *len1, int *cx, int *px)
> +{
> + int *rLenPtr;
> + int rLen;
> + int rPx;
> + int rCx;
> + int rFail;
> +
> + rFail = 0;
> +
> + rCx = smp_load_acquire(cx);
> + rPx = smp_load_acquire(px);
Is it possible for you to put some more comments around which ACQUIRE is
paired with which RELEASE? And, in general more comments around the reason
for a certain memory barrier and what pairs with what. In the kernel sources,
the barriers needs a comment anyway.
> + if (rCx < rPx) {
> + if (rCx == 0) {
> + rLenPtr = len1;
> + } else {
> + rLenPtr = lenFail;
> + rFail = 1;
> + }
> +
> + rLen = smp_load_acquire(rLenPtr);
> + if (rLen == 0) {
> + rFail = 1;
> + } else if (rLen == 1) {
> + rCx = rCx + 1;
> + smp_store_release(cx, rCx);
> + }
> + }
> +}
> +
> +P1(int *lenFail, int *len1, spinlock_t *rb_lock, int *px, int *cx, atomic_t
> *dropped)
> +{
> + int rPx;
> + int rCx;
> + int rFail;
> + int *rLenPtr;
> +
> + rFail = 0;
> +
> + rCx = smp_load_acquire(cx);
> + spin_lock(rb_lock);
> +
> + rPx = *px;
> + if (rPx - rCx >= 1) {
> + atomic_inc(dropped);
Why does 'dropped' need to be atomic if you are always incrementing under a
lock?
> + spin_unlock(rb_lock);
> + } else {
> + if (rPx == 0) {
> + rLenPtr = len1;
> + } else {
> + rLenPtr = lenFail;
> + rFail = 1;
> + }
> +
> + *rLenPtr = -1;
Clarify please the need to set the length intermittently to -1. Thanks.
> + smp_store_release(px, rPx + 1);
> +
> + spin_unlock(rb_lock);
> +
> + smp_store_release(rLenPtr, 1);
> + }
> +}
> +
> +exists (
> + 0:rFail=0 /\ 1:rFail=0
> + /\
> + (
> + (dropped=0 /\ px=1 /\ len1=1 /\ (cx=0 \/ cx=1))
> + )
> +)
> diff --git a/Documentation/litmus-tests/bpf-rb/bpf-rb+1p1c+unbound.litmus
> b/Documentation/litmus-tests/bpf-rb/bpf-rb+1p1c+unbound.litmus
> new file mode 100644
> index 000000000000..7ab5d0e6e49f
> --- /dev/null
> +++ b/Documentation/litmus-tests/bpf-rb/bpf-rb+1p1c+unbound.litmus
I wish there was a way to pass args to litmus tests, then perhaps it would
have been possible to condense some of these tests. :-)
> diff --git a/Documentation/litmus-tests/bpf-rb/bpf-rb+2p1c+bounded.litmus
> b/Documentation/litmus-tests/bpf-rb/bpf-rb+2p1c+bounded.litmus
> new file mode 100644
> index 000000000000..83f80328c92b
> --- /dev/null
> +++ b/Documentation/litmus-tests/bpf-rb/bpf-rb+2p1c+bounded.litmus
[...]
> +P0(int *lenFail, int *len1, int *cx, int *px)
> +{
> + int *rLenPtr;
> + int rLen;
> + int rPx;
> + int rCx;
> + int rFail;
> +
> + rFail = 0;
> +
> + rCx = smp_load_acquire(cx);
> + rPx = smp_load_acquire(px);
> + if (rCx < rPx) {
> + if (rCx == 0) {
> + rLenPtr = len1;
> + } else if (rCx == 1) {
> + rLenPtr = len1;
> + } else {
> + rLenPtr = lenFail;
> + rFail = 1;
> + }
> +
> + rLen = smp_load_acquire(rLenPtr);
> + if (rLen == 0) {
> + rFail = 1;
> + } else if (rLen == 1) {
> + rCx = rCx + 1;
> + smp_store_release(cx, rCx);
> + }
> + }
> +
> + rPx = smp_load_acquire(px);
> + if (rCx < rPx) {
> + if (rCx == 0) {
> + rLenPtr = len1;
> + } else if (rCx == 1) {
> + rLenPtr = len1;
> + } else {
> + rLenPtr = lenFail;
> + rFail = 1;
> + }
> +
> + rLen = smp_load_acquire(rLenPtr);
> + if (rLen == 0) {
> + rFail = 1;
> + } else if (rLen == 1) {
> + rCx = rCx + 1;
> + smp_store_release(cx, rCx);
> + }
> + }
> +}
> +
> +P1(int *lenFail, int *len1, spinlock_t *rb_lock, int *px, int *cx, atomic_t
> *dropped)
> +{
> + int rPx;
> + int rCx;
> + int rFail;
> + int *rLenPtr;
> +
> + rFail = 0;
> + rLenPtr = lenFail;
> +
> + rCx = smp_load_acquire(cx);
> + spin_lock(rb_lock);
> +
> + rPx = *px;
> + if (rPx - rCx >= 1) {
> + atomic_inc(dropped);
> + spin_unlock(rb_lock);
> + } else {
> + if (rPx == 0) {
> + rLenPtr = len1;
> + } else if (rPx == 1) {
> + rLenPtr = len1;
> + } else {
> + rLenPtr = lenFail;
> + rFail = 1;
> + }
> +
> + *rLenPtr = -1;
> + smp_store_release(px, rPx + 1);
> +
> + spin_unlock(rb_lock);
> +
> + smp_store_release(rLenPtr, 1);
I ran a test replacing the last 2 statements above with the following and it
still works:
spin_unlock(rb_lock);
WRITE_ONCE(*rLenPtr, 1);
Wouldn't you expect the test to catch an issue? The spin_unlock is already a
RELEASE barrier.
Suggestion: It is hard to review the patch because it is huge, it would be
good to split this up into 4 patches for each of the tests. But upto you :)
thanks,
- Joel
[...]
