[v1,3/6] test: fence: Fix race condition

19948 diff mbox series

The fence test is racy, as it relies on the main loop being executed
between completion of request signalledRequestId_ and
signalledRequestId_ + 1. This usually happens, but is not guaranteed.

To fix the race condition, change the request identification logic by
replacing usage of the cookie value, is zero-based and wraps around at
nbuffers_ - 1, with a completed request counter that is one-based and
doesn't wrap. The completedRequestId_, expiredRequestId_ and
signalledRequestId_ variables now track the identifier of the last
request that has completed, the request whose fence will time out, and
the request whose fence will be signalled.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 test/fence.cpp | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Quoting Laurent Pinchart (2024-04-25 00:42:21)
> The fence test is racy, as it relies on the main loop being executed
> between completion of request signalledRequestId_ and
> signalledRequestId_ + 1. This usually happens, but is not guaranteed.
> 
> To fix the race condition, change the request identification logic by
> replacing usage of the cookie value, is zero-based and wraps around at

Do you mean "which is zero-based"

> nbuffers_ - 1, with a completed request counter that is one-based and
> doesn't wrap. The completedRequestId_, expiredRequestId_ and
> signalledRequestId_ variables now track the identifier of the last
> request that has completed, the request whose fence will time out, and
> the request whose fence will be signalled.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  test/fence.cpp | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/test/fence.cpp b/test/fence.cpp
> index e6f79d2faa21..7949bfbb176b 100644
> --- a/test/fence.cpp
> +++ b/test/fence.cpp
> @@ -57,8 +57,7 @@ private:
>         bool expectedCompletionResult_ = true;
>         bool setFence_ = true;
>  
> -       unsigned int completedRequest_;
> -
> +       unsigned int completedRequestId_;
>         unsigned int signalledRequestId_;
>         unsigned int expiredRequestId_;
>         unsigned int nbuffers_;
> @@ -126,8 +125,9 @@ int FenceTest::init()
>                 return TestFail;
>         }
>  
> -       signalledRequestId_ = nbuffers_ - 2;
> -       expiredRequestId_ = nbuffers_ - 1;
> +       completedRequestId_ = 0;
> +       expiredRequestId_ = nbuffers_;
> +       signalledRequestId_ = nbuffers_ * 2;

I don't (yet) understand hy this is nbuffers_ * 2 ... while before it
was nbuffers_ - 2 ... that stands out a lot ... but maybe reading
further down will explain.

Maybe that makes it worthy of a comment above to say why those values /
id targets are chosen.

I've read down, and the * 2 isn't clearer. Perhaps it's just to make
sure that enough requests are queued?

>  
>         return TestPass;
>  }
> @@ -189,16 +189,16 @@ void FenceTest::requestRequeue(Request *request)
>         const Request::BufferMap &buffers = request->buffers();
>         const Stream *stream = buffers.begin()->first;
>         FrameBuffer *buffer = buffers.begin()->second;
> -       uint64_t cookie = request->cookie();
>  
>         request->reuse();
>  
> -       if (cookie == signalledRequestId_ && setFence_) {
> +       if (completedRequestId_ == signalledRequestId_ - nbuffers_ && setFence_) {
>                 /*
> -                * The second time this request is queued add a fence to it.
> -                *
> -                * The main loop signals it by using a timer to write to the
> -                * efd2_ file descriptor before the fence expires.
> +                * This is the request that will be used to test fence
> +                * signalling when it completes next time. Add a fence to it,
> +                * using efd2_. The main loop will signal the fence by using a
> +                * timer to write to the efd2_ file descriptor before the fence
> +                * expires.
>                  */
>                 std::unique_ptr<Fence> fence =
>                         std::make_unique<Fence>(std::move(eventFd2_));
> @@ -213,8 +213,7 @@ void FenceTest::requestRequeue(Request *request)
>  
>  void FenceTest::requestComplete(Request *request)
>  {
> -       uint64_t cookie = request->cookie();
> -       completedRequest_ = cookie;
> +       completedRequestId_++;
>  
>         /*
>          * The last request is expected to fail as its fence has not been
> @@ -222,7 +221,7 @@ void FenceTest::requestComplete(Request *request)
>          *
>          * Validate the fence status but do not re-queue it.
>          */
> -       if (cookie == expiredRequestId_) {
> +       if (completedRequestId_ == expiredRequestId_) {
>                 if (validateExpiredRequest(request) != TestPass)
>                         expectedCompletionResult_ = false;
>  
> @@ -271,7 +270,7 @@ int FenceTest::run()
>                 }
>  
>                 int ret;
> -               if (i == expiredRequestId_) {
> +               if (i == expiredRequestId_ - 1) {
>                         /* This request will have a fence, and it will expire. */
>                         std::unique_ptr<Fence> fence =
>                                 std::make_unique<Fence>(std::move(eventFd_));
> @@ -318,7 +317,7 @@ int FenceTest::run()
>         Timer timer;
>         timer.start(1000ms);
>         while (timer.isRunning() && expectedCompletionResult_) {
> -               if (completedRequest_ == signalledRequestId_ && setFence_)
> +               if (completedRequestId_ == signalledRequestId_ - 1 && setFence_)
>                         /*
>                          * signalledRequestId_ has just completed and it has
>                          * been re-queued with a fence. Start the timer to
> -- 
> Regards,
> 
> Laurent Pinchart
>

On Wed, May 29, 2024 at 03:09:22PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2024-04-25 00:42:21)
> > The fence test is racy, as it relies on the main loop being executed
> > between completion of request signalledRequestId_ and
> > signalledRequestId_ + 1. This usually happens, but is not guaranteed.
> > 
> > To fix the race condition, change the request identification logic by
> > replacing usage of the cookie value, is zero-based and wraps around at
> 
> Do you mean "which is zero-based"
> 
> > nbuffers_ - 1, with a completed request counter that is one-based and
> > doesn't wrap. The completedRequestId_, expiredRequestId_ and
> > signalledRequestId_ variables now track the identifier of the last
> > request that has completed, the request whose fence will time out, and
> > the request whose fence will be signalled.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  test/fence.cpp | 29 ++++++++++++++---------------
> >  1 file changed, 14 insertions(+), 15 deletions(-)
> > 
> > diff --git a/test/fence.cpp b/test/fence.cpp
> > index e6f79d2faa21..7949bfbb176b 100644
> > --- a/test/fence.cpp
> > +++ b/test/fence.cpp
> > @@ -57,8 +57,7 @@ private:
> >         bool expectedCompletionResult_ = true;
> >         bool setFence_ = true;
> >  
> > -       unsigned int completedRequest_;
> > -
> > +       unsigned int completedRequestId_;
> >         unsigned int signalledRequestId_;
> >         unsigned int expiredRequestId_;
> >         unsigned int nbuffers_;
> > @@ -126,8 +125,9 @@ int FenceTest::init()
> >                 return TestFail;
> >         }
> >  
> > -       signalledRequestId_ = nbuffers_ - 2;
> > -       expiredRequestId_ = nbuffers_ - 1;
> > +       completedRequestId_ = 0;
> > +       expiredRequestId_ = nbuffers_;
> > +       signalledRequestId_ = nbuffers_ * 2;
> 
> I don't (yet) understand hy this is nbuffers_ * 2 ... while before it
> was nbuffers_ - 2 ... that stands out a lot ... but maybe reading
> further down will explain.
> 
> Maybe that makes it worthy of a comment above to say why those values /
> id targets are chosen.
> 
> I've read down, and the * 2 isn't clearer. Perhaps it's just to make
> sure that enough requests are queued?

I also had trouble with this code, so I think it's worth a comment
indeed. I'll send a v2.

> >  
> >         return TestPass;
> >  }
> > @@ -189,16 +189,16 @@ void FenceTest::requestRequeue(Request *request)
> >         const Request::BufferMap &buffers = request->buffers();
> >         const Stream *stream = buffers.begin()->first;
> >         FrameBuffer *buffer = buffers.begin()->second;
> > -       uint64_t cookie = request->cookie();
> >  
> >         request->reuse();
> >  
> > -       if (cookie == signalledRequestId_ && setFence_) {
> > +       if (completedRequestId_ == signalledRequestId_ - nbuffers_ && setFence_) {
> >                 /*
> > -                * The second time this request is queued add a fence to it.
> > -                *
> > -                * The main loop signals it by using a timer to write to the
> > -                * efd2_ file descriptor before the fence expires.
> > +                * This is the request that will be used to test fence
> > +                * signalling when it completes next time. Add a fence to it,
> > +                * using efd2_. The main loop will signal the fence by using a
> > +                * timer to write to the efd2_ file descriptor before the fence
> > +                * expires.
> >                  */
> >                 std::unique_ptr<Fence> fence =
> >                         std::make_unique<Fence>(std::move(eventFd2_));
> > @@ -213,8 +213,7 @@ void FenceTest::requestRequeue(Request *request)
> >  
> >  void FenceTest::requestComplete(Request *request)
> >  {
> > -       uint64_t cookie = request->cookie();
> > -       completedRequest_ = cookie;
> > +       completedRequestId_++;
> >  
> >         /*
> >          * The last request is expected to fail as its fence has not been
> > @@ -222,7 +221,7 @@ void FenceTest::requestComplete(Request *request)
> >          *
> >          * Validate the fence status but do not re-queue it.
> >          */
> > -       if (cookie == expiredRequestId_) {
> > +       if (completedRequestId_ == expiredRequestId_) {
> >                 if (validateExpiredRequest(request) != TestPass)
> >                         expectedCompletionResult_ = false;
> >  
> > @@ -271,7 +270,7 @@ int FenceTest::run()
> >                 }
> >  
> >                 int ret;
> > -               if (i == expiredRequestId_) {
> > +               if (i == expiredRequestId_ - 1) {
> >                         /* This request will have a fence, and it will expire. */
> >                         std::unique_ptr<Fence> fence =
> >                                 std::make_unique<Fence>(std::move(eventFd_));
> > @@ -318,7 +317,7 @@ int FenceTest::run()
> >         Timer timer;
> >         timer.start(1000ms);
> >         while (timer.isRunning() && expectedCompletionResult_) {
> > -               if (completedRequest_ == signalledRequestId_ && setFence_)
> > +               if (completedRequestId_ == signalledRequestId_ - 1 && setFence_)
> >                         /*
> >                          * signalledRequestId_ has just completed and it has
> >                          * been re-queued with a fence. Start the timer to