[libcamera-devel] [RFC PATCH] libcamera: base: Make the registration of log categories atomic
Nicolas Dufresne
nicolas at ndufresne.ca
Wed Mar 22 16:43:17 CET 2023
Le mardi 21 mars 2023 à 19:50 +0200, Laurent Pinchart a écrit :
> On Tue, Mar 21, 2023 at 05:44:38PM +0000, Kieran Bingham via libcamera-devel wrote:
> > Quoting Nicolas Dufresne via libcamera-devel (2023-03-14 13:00:46)
> > > Le mardi 14 mars 2023 à 14:50 +0530, Umang Jain a écrit :
> > > > Hi Nicolas
> > > >
> > > > On 3/14/23 2:11 AM, Nicolas Dufresne via libcamera-devel wrote:
> > > > > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > > >
> > > > > Logger:::create() is not currently thread safe and causes crashes
> > >
> > > Spotting some of my own typos now, should have two :: here.
> > >
> > > >
> > > > Indeed. Given the fact that gstlibcamerasrc should be ideally moving
> > > > away from using libcamera_private dependency (as LogCategory::create())
> > > > is an internal API
> > >
> > > gstlibcamerasrc is using GStreamer logger only. Perhaps I'm missing something ?
> > > You'll see in the backtrace that libgstcamerasrc is not involved.
> > >
> > > >
> > > > I can see that it's being called from other internal threads as well
> > > > (like PostProcessorWorker from android HAL layer) so making it thread
> > > > safe makes sense.
> > >
> > > Indeed, adding some sort of safety, but resorting to a global lock might not be
> > > the best solution, that's why I started with an RFC.
> >
> > A cursory look through, I don't think this is a problem. This is likely
> > as fine-grained as you can get.
> >
> > I expect the s/:::/::/ can be fixed when applying.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> The create() function is only called when log categories are created,
> which is on first use. The lock contention shouldn't be a big deal, it's
> good enough for now.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Thanks for the reviews, so feel free to fix my tripped : typo and drop the RFC.
regards,
Nicolas
>
> > > > > noticeable on RaspberryPi 4. This adds a mutex around the creation
> > > > > of categories.
> > > > >
> > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > > > ---
> > > > >
> > > > > src/libcamera/base/log.cpp | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > This is a quick fix. The crash I'm getting is very frequent, the backtrace
> > > > > is the following. Pretty much 50% of the time on Raspberry Pi using:
> > > > >
> > > > > gst-launch-1.0 libcamerasrc ! queue ! xvimagesink
> > > > >
> > > > > Thread 6 "gst-launch-1.0" received signal SIGSEGV, Segmentation fault.
> > > > > [Switching to Thread 0x7ff48cc180 (LWP 88100)]
> > > > > 0x0000007ff5f48a40 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::compare(char const*) const () from /lib/aarch64-linux-gnu/libstdc++.so.6
> > > > > (gdb) bt
> > > > > #0 0x0000007ff5f48a40 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::compare(char const*) const () at /lib/aarch64-linux-gnu/libstdc++.so.6
> > > > > #1 0x0000007ff664efe0 in std::operator==<char, std::char_traits<char>, std::allocator<char> >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, char const*)
> > > > > (__lhs=<error reading variable: Cannot access memory at address 0x705f317073696b7a>, __rhs=0x7ff61c4a28 "Event") at /usr/include/c++/10/bits/basic_string.h:6187
> > > > > #2 0x0000007ff61b48d4 in operator()<libcamera::LogCategory*>(libcamera::LogCategory*) const (__closure=0x7ff48cb008, c=0x705f317073696b72) at ../src/libcamera/base/log.cpp:748
> > > > > #3 0x0000007ff61b4910 in __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> >::operator()<__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*> > >(__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >)
> > > > > (this=0x7ff48cb008, __it=0x705f317073696b72) at /usr/include/c++/10/bits/predefined_ops.h:316
> > > > > #4 0x0000007ff61b487c in std::__find_if<__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*> >, __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> > >(__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> >, std::random_access_iterator_tag) (__first=0x705f317073696b72, __last=0x303100736d617261, __pred=...) at /usr/include/c++/10/bits/stl_algobase.h:1959
> > > > > #5 0x0000007ff61b46b4 in std::__find_if<__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*> >, __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> > >(__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__ops::_Iter_pred<libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> >)
> > > > > (__first=0x555582c130, __last=0x303100736d617261, __pred=...) at /usr/include/c++/10/bits/stl_algobase.h:1974
> > > > > #6 0x0000007ff61b4648 in std::find_if<__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*> >, libcamera::Logger::findCategory(char const*) const::<lambda(auto:2)> >(__gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, __gnu_cxx::__normal_iterator<libcamera::LogCategory* const*, std::vector<libcamera::LogCategory*, std::allocator<libcamera::LogCategory*> > >, struct {...}) (__first=0x555582c130, __last=0x303100736d617261, __pred=...) at /usr/include/c++/10/bits/stl_algo.h:3929
> > > > > #7 0x0000007ff61b3f60 in libcamera::Logger::findCategory(char const*) const (this=0x7ff61ec968 <libcamera::Logger::instance()::instance>, name=0x7ff61c4a28 "Event") at ../src/libcamera/base/log.cpp:748
> > > > > #8 0x0000007ff61b3fc4 in libcamera::LogCategory::create(char const*) (name=0x7ff61c4a28 "Event") at ../src/libcamera/base/log.cpp:791
> > > > > #9 0x0000007ff61a9ffc in libcamera::logCategoryEvent() () at ../src/libcamera/base/event_dispatcher.cpp:17
> > > > > #10 0x0000007ff61aac5c in libcamera::EventDispatcherPoll::poll(std::vector<pollfd, std::allocator<pollfd> >*) (this=0x7fe8006f90, pollfds=0x7ff48cb3e8) at ../src/libcamera/base/event_dispatcher_poll.cpp:216
> > > > > #11 0x0000007ff61aa8fc in libcamera::EventDispatcherPoll::processEvents() (this=0x7fe8006f90) at ../src/libcamera/base/event_dispatcher_poll.cpp:160
> > > > > #12 0x0000007ff61be0bc in libcamera::Thread::exec() (this=0x55555d0380) at ../src/libcamera/base/thread.cpp:341
> > > > > #13 0x0000007ff6643e88 in libcamera::CameraManager::Private::run() (this=0x55555d0370) at ../src/libcamera/camera_manager.cpp:122
> > > > > #14 0x0000007ff61be038 in libcamera::Thread::startThread() (this=0x55555d0380) at ../src/libcamera/base/thread.cpp:319
> > > > > #15 0x0000007ff61c2070 in std::__invoke_impl<void, void (libcamera::Thread::*)(), libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&)
> > > > > (__f=@0x5555852ef0: (void (libcamera::Thread::*)(class libcamera::Thread * const)) 0x7ff61bdf38 <libcamera::Thread::startThread()>, __t=@0x5555852ee8: 0x55555d0380) at /usr/include/c++/10/bits/invoke.h:73
> > > > > #16 0x0000007ff61c1f88 in std::__invoke<void (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)
> > > > > (__fn=@0x5555852ef0: (void (libcamera::Thread::*)(class libcamera::Thread * const)) 0x7ff61bdf38 <libcamera::Thread::startThread()>) at /usr/include/c++/10/bits/invoke.h:95
> > > > > #17 0x0000007ff61c1eec in std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) (this=0x5555852ee8)
> > > > > at /usr/include/c++/10/thread:264
> > > > > #18 0x0000007ff61c1ea4 in std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::operator()() (this=0x5555852ee8) at /usr/include/c++/10/thread:271
> > > > > #19 0x0000007ff61c1e84 in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> > >::_M_run() (this=0x5555852ee0) at /usr/include/c++/10/thread:215
> > > > > #20 0x0000007ff5ee1cac in () at /lib/aarch64-linux-gnu/libstdc++.so.6
> > > > > #21 0x0000007ff7c78648 in start_thread (arg=0x7ff48cba80) at pthread_create.c:477
> > > > > #22 0x0000007ff7bcec1c in thread_start () at ../sysdeps/unix/sysv/linux/aarch64/clone.S:78
> > > > >
> > > > >
> > > > > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > > > > index 55fbd7b0..c8045ef7 100644
> > > > > --- a/src/libcamera/base/log.cpp
> > > > > +++ b/src/libcamera/base/log.cpp
> > > > > @@ -21,6 +21,7 @@
> > > > > #include <libcamera/logging.h>
> > > > >
> > > > > #include <libcamera/base/backtrace.h>
> > > > > +#include <libcamera/base/mutex.h>
> > > > > #include <libcamera/base/thread.h>
> > > > > #include <libcamera/base/utils.h>
> > > > >
> > > > > @@ -788,6 +789,8 @@ LogCategory *Logger::findCategory(const char *name) const
> > > > > */
> > > > > LogCategory *LogCategory::create(const char *name)
> > > > > {
> > > > > + static Mutex mutex_;
> > > > > + MutexLocker locker(mutex_);
> > > > > LogCategory *category = Logger::instance()->findCategory(name);
> > > > >
> > > > > if (!category) {
> > > >
> > >
>
More information about the libcamera-devel
mailing list