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

[...]

Reply via email to