You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2020/11/30 23:57:41 UTC
[GitHub] [trafficserver] ywkaras opened a new pull request #7362: Switch to Standard atomic library for free/atomic lists in ink_queue.h.
ywkaras opened a new pull request #7362:
URL: https://github.com/apache/trafficserver/pull/7362
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras removed a comment on pull request #7362: Switch to Standard atomic library for free/atomic lists in ink_queue.h.
Posted by GitBox <gi...@apache.org>.
ywkaras removed a comment on pull request #7362:
URL: https://github.com/apache/trafficserver/pull/7362#issuecomment-737424230
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7362: Replaced by PR #7382
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7362:
URL: https://github.com/apache/trafficserver/pull/7362#issuecomment-742722520
See https://github.com/apache/trafficserver/pull/7382
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7362: Switch to Standard atomic library for free/atomic lists in ink_queue.h.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7362:
URL: https://github.com/apache/trafficserver/pull/7362#issuecomment-737617232
Sigh, although the Au tests pass, this code crashes when I test it with h2load.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7362: Switch to Standard atomic library for free/atomic lists in ink_queue.h.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7362:
URL: https://github.com/apache/trafficserver/pull/7362#discussion_r534617265
##########
File path: src/tscore/ink_queue.cc
##########
@@ -66,6 +67,139 @@
#define DEADBEEF
#endif
+namespace
+{
+// Atomic stack, implemented as a linked list. The link is stored at the offset (given by the base class
+// member function link_offset() ) within each stack element. The stack is empty when the head pointer is null.
+//
+template <class Base> class AtomicStack_ : private Base
+{
+public:
+ template <typename... Args>
+ AtomicStack_(std::atomic<void *> &head, Args &&... args) : Base(std::forward<Args>(args)...), _head(head)
+ {
+ }
+
+ // Return reference to "next" pointer within a stack element.
+ //
+ void *&
+ link(void *elem)
+ {
+ return *reinterpret_cast<void **>(static_cast<char *>(elem) + Base::link_offset());
+ }
+
+ // Splice this stack to the tail of another stack (whose head and tail is given by the parameters).
+ // Returns previous head.
+ //
+ void *
+ splice_to_top(void *other_head, void *other_tail)
+ {
+ void *curr_head = _head;
+
+ do {
+ link(other_tail) = curr_head;
+
+ } while (!_head.compare_exchange_weak(curr_head, other_head));
+
+ return curr_head;
+ }
+
+ // Returns previous head.
+ //
+ void *
+ push(void *elem)
+ {
+ return splice_to_top(elem, elem);
+ }
+
+ template <bool All_elements = false>
+ void *
+ pop()
+ {
+ void *curr_head = _head, *new_head;
+
+ do {
+ if (!curr_head) {
+ // Stack is empty.
+ //
+ return nullptr;
+ }
+
+ new_head = All_elements ? nullptr : link(curr_head);
+
+ } while (!_head.compare_exchange_weak(curr_head, new_head));
+
+ return curr_head;
+ }
+
+ // WARNING: This is not safe, unless no other thread is popping or removing.
+ //
+ bool
+ remove(void *item)
+ {
+ void *p = item;
+ void *p2 = link(item);
+
+ if (_head.compare_exchange_strong(p, p2)) {
+ // Item was at the top of the stack.
+ //
+ return true;
+ }
+ if (p) {
+ p2 = link(p);
+
+ while (p2 != item) {
+ if (nullptr == p2) {
+ // Item not in stack.
+ //
+ return false;
+ }
+ p = p2;
+ p2 = link(p2);
+ }
+ link(p) = link(p2);
+ return true;
+ }
+ // Stack is empty.
+ //
+ return false;
+ }
+
+private:
+ std::atomic<void *> &_head;
+};
+
+class AtomicStackBaseNoOffset
+{
+protected:
+ static constexpr size_t
+ link_offset()
+ {
+ return 0;
+ }
+};
+
+using AtomicStackNoOffset = AtomicStack_<AtomicStackBaseNoOffset>;
+
+class AtomicStackBaseWithOffset
+{
+protected:
+ AtomicStackBaseWithOffset(size_t offset) : _link_offset(offset) {}
+
+ size_t
+ link_offset() const
+ {
+ return (_link_offset);
Review comment:
remove paretheses
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7362: Switch to Standard atomic library for free/atomic lists in ink_queue.h.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7362:
URL: https://github.com/apache/trafficserver/pull/7362#issuecomment-737424230
[approve ci autest]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras closed pull request #7362: Switch to Standard atomic library for free/atomic lists in ink_queue.h.
Posted by GitBox <gi...@apache.org>.
ywkaras closed pull request #7362:
URL: https://github.com/apache/trafficserver/pull/7362
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on pull request #7362: Switch to Standard atomic library for free/atomic lists in ink_queue.h.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on pull request #7362:
URL: https://github.com/apache/trafficserver/pull/7362#issuecomment-740014037
Closing this, as I now understand why the linked list head pointer tagging is necessary ( https://en.wikipedia.org/wiki/ABA_problem ). This code could still use some cleanup, but unlikely to significantly improve performance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] ywkaras commented on a change in pull request #7362: Switch to Standard atomic library for free/atomic lists in ink_queue.h.
Posted by GitBox <gi...@apache.org>.
ywkaras commented on a change in pull request #7362:
URL: https://github.com/apache/trafficserver/pull/7362#discussion_r534617439
##########
File path: src/tscore/ink_queue.cc
##########
@@ -66,6 +67,139 @@
#define DEADBEEF
#endif
+namespace
+{
+// Atomic stack, implemented as a linked list. The link is stored at the offset (given by the base class
+// member function link_offset() ) within each stack element. The stack is empty when the head pointer is null.
+//
+template <class Base> class AtomicStack_ : private Base
+{
+public:
+ template <typename... Args>
+ AtomicStack_(std::atomic<void *> &head, Args &&... args) : Base(std::forward<Args>(args)...), _head(head)
+ {
+ }
+
+ // Return reference to "next" pointer within a stack element.
+ //
+ void *&
+ link(void *elem)
+ {
+ return *reinterpret_cast<void **>(static_cast<char *>(elem) + Base::link_offset());
+ }
+
+ // Splice this stack to the tail of another stack (whose head and tail is given by the parameters).
Review comment:
are given
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org