| Message ID | 20200118213348.16561-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 1d929967b7d5623ec49f0bbed3abe4fea480011e |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your patch. On 2020-01-18 23:33:47 +0200, Laurent Pinchart wrote: > The current thread ID is useful when logging message to debug > concurrency issues. Add a method to retrieve it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/include/thread.h | 2 ++ > src/libcamera/thread.cpp | 20 ++++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h > index 37edd4f5138b..819a9879ac56 100644 > --- a/src/libcamera/include/thread.h > +++ b/src/libcamera/include/thread.h > @@ -9,6 +9,7 @@ > > #include <memory> > #include <mutex> > +#include <sys/types.h> > #include <thread> > > #include <libcamera/signal.h> > @@ -39,6 +40,7 @@ public: > Signal<Thread *> finished; > > static Thread *current(); > + static pid_t currentId(); > > EventDispatcher *eventDispatcher(); > void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher); > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp > index 18ebd16a7e2f..fe32cd677596 100644 > --- a/src/libcamera/thread.cpp > +++ b/src/libcamera/thread.cpp > @@ -9,6 +9,9 @@ > > #include <atomic> > #include <list> > +#include <unistd.h> > +#include <sys/syscall.h> > +#include <sys/types.h> > > #include <libcamera/event_dispatcher.h> > > @@ -62,6 +65,7 @@ private: > > Thread *thread_; > bool running_; > + pid_t tid_; > > Mutex mutex_; > > @@ -108,6 +112,7 @@ ThreadData *ThreadData::current() > * started, set it here. > */ > ThreadData *data = mainThread.data_; > + data->tid_ = syscall(SYS_gettid); > currentThreadData = data; > return data; > } > @@ -189,6 +194,7 @@ void Thread::startThread() > */ > thread_local ThreadCleaner cleaner(this, &Thread::finishThread); > > + data_->tid_ = syscall(SYS_gettid); > currentThreadData = data_; > > run(); > @@ -308,6 +314,20 @@ Thread *Thread::current() > return data->thread_; > } > > +/** > + * \brief Retrieve the ID of the current thread > + * > + * The thread ID corresponds to the Linux thread ID (TID) as returned by the > + * gettid system call. > + * > + * \return The ID of the current thread > + */ > +pid_t Thread::currentId() > +{ > + ThreadData *data = ThreadData::current(); > + return data->tid_; > +} > + > /** > * \brief Set the event dispatcher > * \param[in] dispatcher Pointer to the event dispatcher > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On 18/01/2020 21:33, Laurent Pinchart wrote: > The current thread ID is useful when logging message to debug > concurrency issues. Add a method to retrieve it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/include/thread.h | 2 ++ > src/libcamera/thread.cpp | 20 ++++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h > index 37edd4f5138b..819a9879ac56 100644 > --- a/src/libcamera/include/thread.h > +++ b/src/libcamera/include/thread.h > @@ -9,6 +9,7 @@ > > #include <memory> > #include <mutex> > +#include <sys/types.h> > #include <thread> > > #include <libcamera/signal.h> > @@ -39,6 +40,7 @@ public: > Signal<Thread *> finished; > > static Thread *current(); > + static pid_t currentId(); > > EventDispatcher *eventDispatcher(); > void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher); > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp > index 18ebd16a7e2f..fe32cd677596 100644 > --- a/src/libcamera/thread.cpp > +++ b/src/libcamera/thread.cpp > @@ -9,6 +9,9 @@ > > #include <atomic> > #include <list> > +#include <unistd.h> > +#include <sys/syscall.h> > +#include <sys/types.h> > > #include <libcamera/event_dispatcher.h> > > @@ -62,6 +65,7 @@ private: > > Thread *thread_; > bool running_; > + pid_t tid_; > > Mutex mutex_; > > @@ -108,6 +112,7 @@ ThreadData *ThreadData::current() > * started, set it here. > */ > ThreadData *data = mainThread.data_; > + data->tid_ = syscall(SYS_gettid); > currentThreadData = data; > return data; > } > @@ -189,6 +194,7 @@ void Thread::startThread() > */ > thread_local ThreadCleaner cleaner(this, &Thread::finishThread); > > + data_->tid_ = syscall(SYS_gettid); We will now have direct syscalls from V4L2Device, V4L2VideoDevice, and Thread. Not that we particularly want another-adaptation-layer, but should these be moved to a single layer (or even just utils::?), as effectively they are c-library emulations? Anyway, if we chose to do so, that can be a standalone patch on top. This is certainly a good idea to help threaded debug. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > currentThreadData = data_; > > run(); > @@ -308,6 +314,20 @@ Thread *Thread::current() > return data->thread_; > } > > +/** > + * \brief Retrieve the ID of the current thread > + * > + * The thread ID corresponds to the Linux thread ID (TID) as returned by the > + * gettid system call. > + * > + * \return The ID of the current thread > + */ > +pid_t Thread::currentId() > +{ > + ThreadData *data = ThreadData::current(); > + return data->tid_; > +} > + > /** > * \brief Set the event dispatcher > * \param[in] dispatcher Pointer to the event dispatcher >
Hi Kieran, On Mon, Jan 20, 2020 at 10:11:58AM +0000, Kieran Bingham wrote: > On 18/01/2020 21:33, Laurent Pinchart wrote: > > The current thread ID is useful when logging message to debug > > concurrency issues. Add a method to retrieve it. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/include/thread.h | 2 ++ > > src/libcamera/thread.cpp | 20 ++++++++++++++++++++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h > > index 37edd4f5138b..819a9879ac56 100644 > > --- a/src/libcamera/include/thread.h > > +++ b/src/libcamera/include/thread.h > > @@ -9,6 +9,7 @@ > > > > #include <memory> > > #include <mutex> > > +#include <sys/types.h> > > #include <thread> > > > > #include <libcamera/signal.h> > > @@ -39,6 +40,7 @@ public: > > Signal<Thread *> finished; > > > > static Thread *current(); > > + static pid_t currentId(); > > > > EventDispatcher *eventDispatcher(); > > void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher); > > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp > > index 18ebd16a7e2f..fe32cd677596 100644 > > --- a/src/libcamera/thread.cpp > > +++ b/src/libcamera/thread.cpp > > @@ -9,6 +9,9 @@ > > > > #include <atomic> > > #include <list> > > +#include <unistd.h> > > +#include <sys/syscall.h> > > +#include <sys/types.h> > > > > #include <libcamera/event_dispatcher.h> > > > > @@ -62,6 +65,7 @@ private: > > > > Thread *thread_; > > bool running_; > > + pid_t tid_; > > > > Mutex mutex_; > > > > @@ -108,6 +112,7 @@ ThreadData *ThreadData::current() > > * started, set it here. > > */ > > ThreadData *data = mainThread.data_; > > + data->tid_ = syscall(SYS_gettid); > > currentThreadData = data; > > return data; > > } > > @@ -189,6 +194,7 @@ void Thread::startThread() > > */ > > thread_local ThreadCleaner cleaner(this, &Thread::finishThread); > > > > + data_->tid_ = syscall(SYS_gettid); > > We will now have direct syscalls from V4L2Device, V4L2VideoDevice, and > Thread. > > Not that we particularly want another-adaptation-layer, but should these > be moved to a single layer (or even just utils::?), as effectively they > are c-library emulations? For the openat() syscall, we may want to do so, as it may need to be wrapped with additional logic (for instance to handle 32/64-bit file access, if needed). For gettid here, I'm not sure, as there's no gettid() in C libraries. And now that I've written this, I realize that glibc has added a gettid() wrapper in v2.30 (released on August the 1st 2019). We could thus add a wrapper too, but more as a convenience than anything else, as it's just one syscall without any glue around it. > Anyway, if we chose to do so, that can be a standalone patch on top. > > This is certainly a good idea to help threaded debug. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > currentThreadData = data_; > > > > run(); > > @@ -308,6 +314,20 @@ Thread *Thread::current() > > return data->thread_; > > } > > > > +/** > > + * \brief Retrieve the ID of the current thread > > + * > > + * The thread ID corresponds to the Linux thread ID (TID) as returned by the > > + * gettid system call. > > + * > > + * \return The ID of the current thread > > + */ > > +pid_t Thread::currentId() > > +{ > > + ThreadData *data = ThreadData::current(); > > + return data->tid_; > > +} > > + > > /** > > * \brief Set the event dispatcher > > * \param[in] dispatcher Pointer to the event dispatcher
diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h index 37edd4f5138b..819a9879ac56 100644 --- a/src/libcamera/include/thread.h +++ b/src/libcamera/include/thread.h @@ -9,6 +9,7 @@ #include <memory> #include <mutex> +#include <sys/types.h> #include <thread> #include <libcamera/signal.h> @@ -39,6 +40,7 @@ public: Signal<Thread *> finished; static Thread *current(); + static pid_t currentId(); EventDispatcher *eventDispatcher(); void setEventDispatcher(std::unique_ptr<EventDispatcher> dispatcher); diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp index 18ebd16a7e2f..fe32cd677596 100644 --- a/src/libcamera/thread.cpp +++ b/src/libcamera/thread.cpp @@ -9,6 +9,9 @@ #include <atomic> #include <list> +#include <unistd.h> +#include <sys/syscall.h> +#include <sys/types.h> #include <libcamera/event_dispatcher.h> @@ -62,6 +65,7 @@ private: Thread *thread_; bool running_; + pid_t tid_; Mutex mutex_; @@ -108,6 +112,7 @@ ThreadData *ThreadData::current() * started, set it here. */ ThreadData *data = mainThread.data_; + data->tid_ = syscall(SYS_gettid); currentThreadData = data; return data; } @@ -189,6 +194,7 @@ void Thread::startThread() */ thread_local ThreadCleaner cleaner(this, &Thread::finishThread); + data_->tid_ = syscall(SYS_gettid); currentThreadData = data_; run(); @@ -308,6 +314,20 @@ Thread *Thread::current() return data->thread_; } +/** + * \brief Retrieve the ID of the current thread + * + * The thread ID corresponds to the Linux thread ID (TID) as returned by the + * gettid system call. + * + * \return The ID of the current thread + */ +pid_t Thread::currentId() +{ + ThreadData *data = ThreadData::current(); + return data->tid_; +} + /** * \brief Set the event dispatcher * \param[in] dispatcher Pointer to the event dispatcher
The current thread ID is useful when logging message to debug concurrency issues. Add a method to retrieve it. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/include/thread.h | 2 ++ src/libcamera/thread.cpp | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+)