You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/03/28 21:02:59 UTC
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6502
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
MemTracker::LogUsage() and MemTracker::EnableReservationReporting()
could race, with LogUsage() seeing a partially-constructed
'reservation_counters_' value and crashing.
This patch fixes that issue by atomically swapping in
'reservation_counters_' so that no threads see a
partially-constructed value.
While we're here, swap boost::mutex for the higher-performance
SpinLock.
Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
---
M be/src/common/atomic.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
3 files changed, 37 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/6502/1
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 1: Code-Review+1
(5 comments)
http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/common/atomic.h
File be/src/common/atomic.h:
Line 122: public:
indentation
http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:
Line 123: ReservationTrackerCounters* new_counters = new ReservationTrackerCounters;
is there an compiler-generated copy-c'tor for ReservationTrackerCounters?
PS1, Line 198: delete
it's really tempting to have the AtomicPtr delete its contents on destruction, but I guess we should make a separate AtomicScopedPtr if we have more use cases for it.
PS1, Line 307: gc_functions_[i]();
if these are caller-supplied, should the interface say something about how they should be non-blocking, since they're executed while holding a spinlock?
http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
PS1, Line 386: Must be read and written atomically
I'd remove this. Or say "may be read concurrently". It's not clear how a reader could arrange to read this atomically (unless it CAS-es the pointer out of the AtomicPtr?), and I'm not sure that's what the comment means to say.
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6502
to look at the new patch set (#2).
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
MemTracker::LogUsage() and MemTracker::EnableReservationReporting()
could race, with LogUsage() seeing a partially-constructed
'reservation_counters_' value and crashing.
This patch fixes that issue by atomically swapping in
'reservation_counters_' so that no threads see a
partially-constructed value.
While we're here, swap boost::mutex for the higher-performance
SpinLock for 'child_trackers_lock_'.
Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
---
M be/src/common/atomic.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
3 files changed, 38 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/6502/2
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 3:
Before we were assigning the scoped_ptr then initialising the memory, so the uninitialised or partially initialised structure was visible to other threads. ReservationTrackerCounters is a plain struct so isn't zero-initialised or anything.
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6502
to look at the new patch set (#3).
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
MemTracker::LogUsage() and MemTracker::EnableReservationReporting()
could race, with LogUsage() seeing a partially-constructed
'reservation_counters_' value and crashing.
This patch fixes that issue by atomically swapping in
'reservation_counters_' so that no threads see a
partially-constructed value.
While we're here, swap boost::mutex for the higher-performance
SpinLock for 'child_trackers_lock_'.
Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
---
M be/src/common/atomic.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
3 files changed, 38 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/6502/3
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 3:
Agree that this could probably be improved if we reworked how MemTrackers/ReservationTrackers were set up but I didn't see a non-invasive way.
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 1: Code-Review+1
(5 comments)
Carry +1
http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/common/atomic.h
File be/src/common/atomic.h:
Line 122: public:
> indentation
Done
http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:
Line 123: ReservationTrackerCounters* new_counters = new ReservationTrackerCounters;
> is there an compiler-generated copy-c'tor for ReservationTrackerCounters?
Done
PS1, Line 198: delete
> it's really tempting to have the AtomicPtr delete its contents on destructi
I thought about that too, but it complicates the implementation a bit to have an interface analogous to scoped_ptr - e.g. you have to implement a Store() as a swap and delete to handle the case when the value is already non-null.
PS1, Line 307: gc_functions_[i]();
> if these are caller-supplied, should the interface say something about how
I added a bit more description of what the gc_functions_ should do. The current SpinLock implementation now uses gutil's spinlock, which falls back to a kernel futex after a while, so we don't have the same problem as with the old SpinLock.
That said, mutex is still probably a better fit here, since the critical section is pretty long-running and it shouldn't be frequently acquired. The fairness properties may be nice too. So I reverted that part of the change.
http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:
PS1, Line 386: Must be read and written atomically
> I'd remove this. Or say "may be read concurrently". It's not clear how a re
Done.
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 3:
> Before we were assigning the scoped_ptr then initialising the
>
Ah I didn't look at that old code carefully enough.
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6502/2/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:
Line 251: lock_guard<SpinLock> l(child_trackers_lock_);
why do we need to take this lock earlier?
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6502/2/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:
Line 251: lock_guard<SpinLock> l(child_trackers_lock_);
> why do we need to take this lock earlier?
Oops, missed that. That was an accidental change - I was experimenting with a different solution and didn't fully undo my changes.
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 3: Code-Review+2
This is fine, though it would be nice if memtracker/reservationtracker initialization happened in a more controlled and systematic way (i.e. not registered into the tree until fully initialized).
But I'm not sure this really explains the crash you have in the jira. Are you suspecting that the compiler is reordering the construction of the reservation tracker and the write of the scoped_ptr? Or that the scope_ptr read/write is being broken into multiple instructions?
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
MemTracker::LogUsage() and MemTracker::EnableReservationReporting()
could race, with LogUsage() seeing a partially-constructed
'reservation_counters_' value and crashing.
This patch fixes that issue by atomically swapping in
'reservation_counters_' so that no threads see a
partially-constructed value.
While we're here, swap boost::mutex for the higher-performance
SpinLock for 'child_trackers_lock_'.
Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Reviewed-on: http://gerrit.cloudera.org:8080/6502
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/common/atomic.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
3 files changed, 38 insertions(+), 16 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Dan Hecht: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
......................................................................
Patch Set 3:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/421/
--
To view, visit http://gerrit.cloudera.org:8080/6502
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No