Basic ev2 framework #5401
Basic ev2 framework #5401
Conversation
Summary: Extremely rough implementation of the basic componenets to get things going. Blueprint issue osquery#5158 . Reviewed By: akindyakov Differential Revision: D13779295 fbshipit-source-id: b01bf88d61cc8c3241f91e651e3ebab34246e5a9
facebook-github-bot
added
the
cla signed
| * safe to call to call any method on the object as soon as abort() exits. No | ||
| * events will be lost in by calling abort(). | ||
| */ | ||
| void abort() override { |
There was a problem hiding this comment.
hi @fmanco , I believe abort_ should be set under the protection of the lock else there is a race.
{
std::unique_lockstd::mutex lock(buffer_mutex_);
abort_ = true;
buffer_cv_.notify_all(); // keep this inside or outside of the scope your choice
}
There was a problem hiding this comment.
What is the race condition you're seeing?
There was a problem hiding this comment.
My reasoning is simple, abort_ is shared by two (or more ) threads so it should be accessed under the protection of a lock.
I can give you an example where it can cause 'while (waiting_ != 0) {}' to spin for indefinite period of time.
At present abort_ is being used as relaxed-atomic (atomic since on x64 aligned object are atomic in nature).
There was a problem hiding this comment.
Yeah, abort_ is being used as an atomic. Just out of curiosity can you give the example where the abort loop would spin forever?
There was a problem hiding this comment.
Suppose there are two threads
Time line (T0<T1<T2)
thread 1 | thread2
T0 abort_ = true; |
T1 buffer_cv_.notify_all(); |
T2 | buffer_cv_.wait(
lock, [this, batch] { return buffer_.size() >= batch ||
abort_; });
In the above example if condition (buffer_.size() >= batch) is false and decision is to be taken based upon the (abort_==true) then do not assume that abort_ is true since it happened at T0 and it is being checked at time T2. Since abort_ can be false so thread2 can go back to sleep on condition variable. Since waiting_ has value >0 which will cause thread1 to loop.
| buffer_cv_.notify_all(); | ||
|
|
||
| while (waiting_ != 0) { | ||
| /* spinlock */ |
There was a problem hiding this comment.
inside this tight loop.
std::this_thread::yield(); // This can avoid busy waiting.
There was a problem hiding this comment.
But if I just yield how is the thread going to be wake up?
There was a problem hiding this comment.
purpose to suggest for using yield was to avoid busy waiting (since this tight loop can cause cpu spike).
yield is like sleep(0) introduces a context switch.
I thought again an found its not appropriate here.
I think if you really want to wait for (waiting_==0), you should introduce another condition variable associating it with buffer_mutex_. There should not be any busy waiting here.
There was a problem hiding this comment.
Yeah, I agree with that. This is basically a PoC to get things going for us to plug the eBPF system. I'm going to be working on improving the performance of this, and I'll remove the spinlock. Ideally I would like to remove the locks by using some lock-free queue.
| /* spinlock */ | ||
| } | ||
|
|
||
| abort_ = false; |
There was a problem hiding this comment.
lock protection might be also required here
|
@uptycs-nishant Thanks for the review, I totally missed this comments somehow, I'll take a look now and address them. |
Summary: Pull Request resolved: osquery#5401 Extremely rough implementation of the basic componenets to get things going. Blueprint issue osquery#5158 . Reviewed By: akindyakov Differential Revision: D13779295 fbshipit-source-id: c7373794e8152ffea8a7c5d97f0c937bf97a2a0a
Summary:
Extremely rough implementation of the basic componenets to get things going.
Blueprint issue #5158 .
Reviewed By: akindyakov
Differential Revision: D13779295