[v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need not be atomic
diff mbox series

Message ID 20250310170326.185273-1-barnabas.pocze@ideasonboard.com
State Accepted
Headers show
Series
  • [v1] libcamera: v4l2_videodevice: `lastUsedCounter_` need not be atomic
Related show

Commit Message

Barnabás Pőcze March 10, 2025, 5:03 p.m. UTC 
The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`
member is not used in contexts where its atomicity would matter.
So it does not need to be have an atomic type.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/internal/v4l2_videodevice.h | 2 +-
 src/libcamera/v4l2_videodevice.cpp            | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Jacopo Mondi March 17, 2025, 6:33 p.m. UTC | #1 
Hi Barnabás

On Mon, Mar 10, 2025 at 06:03:26PM +0100, Barnabás Pőcze wrote:
> The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`
> member is not used in contexts where its atomicity would matter.
> So it does not need to be have an atomic type.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

This had me do more reasoning than expected, so let me see if I got
this right.

The buffer cached is accessed at buffer queue time (get()) and at
buffer dequeue time (put()). Dequeue is an async event generated by
the video device while buffer queuing is triggered by the application.

dequeue is triggered by an event dispatcher, while queueRequest runs in
the pipeline handler thread (which is the same thread where
all other Object (but Threads) run in libcamera).

As the event dispatcher is setEnabled(true) at the first call to
queueBuffers, the thread it runs on is again the pipeline handler thread.

So yeah, seems safe indeed

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> ---
>  include/libcamera/internal/v4l2_videodevice.h | 2 +-
>  src/libcamera/v4l2_videodevice.cpp            | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index f021c2a01..e52b50c67 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -158,7 +158,7 @@ private:
>  		std::vector<Plane> planes_;
>  	};
>
> -	std::atomic<uint64_t> lastUsedCounter_;
> +	uint64_t lastUsedCounter_;
>  	std::vector<Entry> cache_;
>  	/* \todo Expose the miss counter through an instrumentation API. */
>  	unsigned int missCounter_;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index e241eb47b..f5b3fa09d 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -190,7 +190,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>
>  {
>  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
>  		cache_.emplace_back(true,
> -				    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> +				    lastUsedCounter_++,
>  				    *buffer.get());
>  }
>
> @@ -258,7 +258,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>  		return -ENOENT;
>
>  	cache_[use] = Entry(false,
> -			    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> +			    lastUsedCounter_++,
>  			    buffer);
>
>  	return use;
> --
> 2.48.1
>
Kieran Bingham March 19, 2025, 2:53 p.m. UTC | #2 
Quoting Barnabás Pőcze (2025-03-10 17:03:26)
> The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`
> member is not used in contexts where its atomicity would matter.
> So it does not need to be have an atomic type.

Any indications as to why this was using atomic ? Maybe just
cautious development when it was first put in ?

As long as we're convinced it's still safe and passes the tests, I'm
fine with it. It's certainly simpler code.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h | 2 +-
>  src/libcamera/v4l2_videodevice.cpp            | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index f021c2a01..e52b50c67 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -158,7 +158,7 @@ private:
>                 std::vector<Plane> planes_;
>         };
>  
> -       std::atomic<uint64_t> lastUsedCounter_;
> +       uint64_t lastUsedCounter_;
>         std::vector<Entry> cache_;
>         /* \todo Expose the miss counter through an instrumentation API. */
>         unsigned int missCounter_;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index e241eb47b..f5b3fa09d 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -190,7 +190,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>
>  {
>         for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
>                 cache_.emplace_back(true,
> -                                   lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> +                                   lastUsedCounter_++,
>                                     *buffer.get());
>  }
>  
> @@ -258,7 +258,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>                 return -ENOENT;
>  
>         cache_[use] = Entry(false,
> -                           lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> +                           lastUsedCounter_++,
>                             buffer);
>  
>         return use;
> -- 
> 2.48.1
>
Barnabás Pőcze March 19, 2025, 3:34 p.m. UTC | #3 
Hi


2025. 03. 19. 15:53 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-03-10 17:03:26)
>> The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`
>> member is not used in contexts where its atomicity would matter.
>> So it does not need to be have an atomic type.
> 
> Any indications as to why this was using atomic ? Maybe just
> cautious development when it was first put in ?

Not sure. It was Laurent who suggested it: https://patchwork.libcamera.org/patch/2878/#3889

There is only a single place where `lastUsedCounter_` might be used
by multiple threads: `V4L2BufferCache::get()`. But that function
is not thread safe, so either `lastUsedCounter_` is unnecessarily
atomic or race conditions are rampant already. As far as I can
tell it is the former.


Regards,
Barnabás Pőcze


> 
> As long as we're convinced it's still safe and passes the tests, I'm
> fine with it. It's certainly simpler code.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   include/libcamera/internal/v4l2_videodevice.h | 2 +-
>>   src/libcamera/v4l2_videodevice.cpp            | 4 ++--
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
>> index f021c2a01..e52b50c67 100644
>> --- a/include/libcamera/internal/v4l2_videodevice.h
>> +++ b/include/libcamera/internal/v4l2_videodevice.h
>> @@ -158,7 +158,7 @@ private:
>>                  std::vector<Plane> planes_;
>>          };
>>   
>> -       std::atomic<uint64_t> lastUsedCounter_;
>> +       uint64_t lastUsedCounter_;
>>          std::vector<Entry> cache_;
>>          /* \todo Expose the miss counter through an instrumentation API. */
>>          unsigned int missCounter_;
>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>> index e241eb47b..f5b3fa09d 100644
>> --- a/src/libcamera/v4l2_videodevice.cpp
>> +++ b/src/libcamera/v4l2_videodevice.cpp
>> @@ -190,7 +190,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>
>>   {
>>          for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
>>                  cache_.emplace_back(true,
>> -                                   lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
>> +                                   lastUsedCounter_++,
>>                                      *buffer.get());
>>   }
>>   
>> @@ -258,7 +258,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>>                  return -ENOENT;
>>   
>>          cache_[use] = Entry(false,
>> -                           lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
>> +                           lastUsedCounter_++,
>>                              buffer);
>>   
>>          return use;
>> -- 
>> 2.48.1
>>
Laurent Pinchart March 26, 2025, 2:10 a.m. UTC | #4 
On Wed, Mar 19, 2025 at 04:34:44PM +0100, Barnabás Pőcze wrote:
> 2025. 03. 19. 15:53 keltezéssel, Kieran Bingham írta:
> > Quoting Barnabás Pőcze (2025-03-10 17:03:26)
> >> The `V4L2BufferCache` type is not thread-safe. Its `lastUsedCounter_`
> >> member is not used in contexts where its atomicity would matter.
> >> So it does not need to be have an atomic type.
> > 
> > Any indications as to why this was using atomic ? Maybe just
> > cautious development when it was first put in ?
> 
> Not sure. It was Laurent who suggested it: https://patchwork.libcamera.org/patch/2878/#3889
> 
> There is only a single place where `lastUsedCounter_` might be used
> by multiple threads: `V4L2BufferCache::get()`. But that function
> is not thread safe, so either `lastUsedCounter_` is unnecessarily
> atomic or race conditions are rampant already. As far as I can
> tell it is the former.

Agreed.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > As long as we're convinced it's still safe and passes the tests, I'm
> > fine with it. It's certainly simpler code.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   include/libcamera/internal/v4l2_videodevice.h | 2 +-
> >>   src/libcamera/v4l2_videodevice.cpp            | 4 ++--
> >>   2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> >> index f021c2a01..e52b50c67 100644
> >> --- a/include/libcamera/internal/v4l2_videodevice.h
> >> +++ b/include/libcamera/internal/v4l2_videodevice.h
> >> @@ -158,7 +158,7 @@ private:
> >>                  std::vector<Plane> planes_;
> >>          };
> >>   
> >> -       std::atomic<uint64_t> lastUsedCounter_;
> >> +       uint64_t lastUsedCounter_;
> >>          std::vector<Entry> cache_;
> >>          /* \todo Expose the miss counter through an instrumentation API. */
> >>          unsigned int missCounter_;
> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> >> index e241eb47b..f5b3fa09d 100644
> >> --- a/src/libcamera/v4l2_videodevice.cpp
> >> +++ b/src/libcamera/v4l2_videodevice.cpp
> >> @@ -190,7 +190,7 @@ V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>
> >>   {
> >>          for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
> >>                  cache_.emplace_back(true,
> >> -                                   lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> >> +                                   lastUsedCounter_++,
> >>                                      *buffer.get());
> >>   }
> >>   
> >> @@ -258,7 +258,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
> >>                  return -ENOENT;
> >>   
> >>          cache_[use] = Entry(false,
> >> -                           lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> >> +                           lastUsedCounter_++,
> >>                              buffer);
> >>   
> >>          return use;

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index f021c2a01..e52b50c67 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -158,7 +158,7 @@  private:
 		std::vector<Plane> planes_;
 	};
 
-	std::atomic<uint64_t> lastUsedCounter_;
+	uint64_t lastUsedCounter_;
 	std::vector<Entry> cache_;
 	/* \todo Expose the miss counter through an instrumentation API. */
 	unsigned int missCounter_;
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index e241eb47b..f5b3fa09d 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -190,7 +190,7 @@  V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>>
 {
 	for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
 		cache_.emplace_back(true,
-				    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
+				    lastUsedCounter_++,
 				    *buffer.get());
 }
 
@@ -258,7 +258,7 @@  int V4L2BufferCache::get(const FrameBuffer &buffer)
 		return -ENOENT;
 
 	cache_[use] = Entry(false,
-			    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
+			    lastUsedCounter_++,
 			    buffer);
 
 	return use;