Merge PR #24129 into master

* refs/pull/24129/head:
	common: drop ::length() from other implementations of the OpQueue.
	common, osd: kill OpQueue::length() to minimize inter-core traffic.

Reviewed-by: Adam Kupczyk <akucpzyk@redhat.com>

10 files changed, 69 insertions, 70 deletions

diff --git a/src/common/OpQueue.h b/src/common/OpQueue.h
index 26894b37fd2..db98cbd318f 100644
--- a/src/common/OpQueue.h
+++ b/src/common/OpQueue.h
@@ -35,8 +35,6 @@ template <typename T, typename K>
 class OpQueue {
 
   public:
-    // How many Ops are in the queue
-    virtual unsigned length() const = 0;
     // Ops of this class should be deleted immediately. If out isn't
     // nullptr then items should be added to the front in
     // front-to-back order. The typical strategy is to visit items in
diff --git a/src/common/PrioritizedQueue.h b/src/common/PrioritizedQueue.h
index 2e86d1069ff..6d7de1291f6 100644
--- a/src/common/PrioritizedQueue.h
+++ b/src/common/PrioritizedQueue.h
@@ -202,7 +202,7 @@ public:
       min_cost(min_c)
   {}
 
-  unsigned length() const final {
+  unsigned length() const {
     unsigned total = 0;
     for (typename SubQueues::const_iterator i = queue.begin();
 	 i != queue.end();
diff --git a/src/common/WeightedPriorityQueue.h b/src/common/WeightedPriorityQueue.h
index 91e16ea2aa5..a05174e8185 100644
--- a/src/common/WeightedPriorityQueue.h
+++ b/src/common/WeightedPriorityQueue.h
@@ -104,19 +104,16 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
       unsigned get_size() const {
 	return lp.size();
       }
-      unsigned filter_class(std::list<T>* out) {
-        unsigned count = 0;
+      void filter_class(std::list<T>* out) {
         for (Lit i = --lp.end();; --i) {
           if (out) {
             out->push_front(std::move(i->item));
           }
           i = lp.erase_and_dispose(i, DelItem<ListPair>());
-          ++count;
           if (i == lp.begin()) {
             break;
           }
         }
-        return count;
       }
     };
     class SubQueue : public bi::set_base_hook<>
@@ -172,17 +169,23 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
         check_end();
 	return ret;
       }
-      unsigned filter_class(K& cl, std::list<T>* out) {
-	unsigned count = 0;
+      void filter_class(K& cl, std::list<T>* out) {
         Kit i = klasses.find(cl, MapKey<Klass, K>());
         if (i != klasses.end()) {
-          count = i->filter_class(out);
+          i->filter_class(out);
 	  Kit tmp = klasses.erase_and_dispose(i, DelItem<Klass>());
 	  if (next == i) {
             next = tmp;
           }
           check_end();
         }
+      }
+      // this is intended for unit tests and should be never used on hot paths
+      unsigned get_size_slow() const {
+	unsigned count = 0;
+	for (const auto& klass : klasses) {
+	  count += klass.get_size();
+	}
 	return count;
       }
       void dump(ceph::Formatter *f) const {
@@ -199,17 +202,15 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
       unsigned total_prio;
       unsigned max_cost;
       public:
-	unsigned size;
 	Queue() :
 	  total_prio(0),
-	  max_cost(0),
-	  size(0) {
+	  max_cost(0) {
 	}
 	~Queue() {
 	  queues.clear_and_dispose(DelItem<SubQueue>());
 	}
 	bool empty() const {
-	  return !size;
+	  return queues.empty();
 	}
 	void insert(unsigned p, K cl, unsigned cost, T&& item, bool front = false) {
 	  typename SubQueues::insert_commit_data insert_data;
@@ -223,10 +224,8 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
 	  if (cost > max_cost) {
 	    max_cost = cost;
 	  }
-	  ++size;
 	}
 	T pop(bool strict = false) {
-	  --size;
 	  Sit i = --queues.end();
 	  if (strict) {
 	    T ret = i->pop();
@@ -269,7 +268,7 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
 	}
 	void filter_class(K& cl, std::list<T>* out) {
 	  for (Sit i = queues.begin(); i != queues.end();) {
-	    size -= i->filter_class(cl, out);
+	    i->filter_class(cl, out);
 	    if (i->empty()) {
 	      total_prio -= i->key;
 	      i = queues.erase_and_dispose(i, DelItem<SubQueue>());
@@ -278,6 +277,14 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
 	    }
 	  }
 	}
+	// this is intended for unit tests and should be never used on hot paths
+	unsigned get_size_slow() const {
+	  unsigned count = 0;
+	  for (const auto& queue : queues) {
+	    count += queue.get_size_slow();
+	  }
+	  return count;
+	}
 	void dump(ceph::Formatter *f) const {
 	  for (typename SubQueues::const_iterator i = queues.begin();
 	        i != queues.end(); ++i) {
@@ -300,15 +307,12 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
       {
 	std::srand(time(0));
       }
-    unsigned length() const final {
-      return strict.size + normal.size;
-    }
     void remove_by_class(K cl, std::list<T>* removed = 0) final {
       strict.filter_class(cl, removed);
       normal.filter_class(cl, removed);
     }
     bool empty() const final {
-      return !(strict.size + normal.size);
+      return strict.empty() && normal.empty();
     }
     void enqueue_strict(K cl, unsigned p, T&& item) final {
       strict.insert(p, cl, 0, std::move(item));
@@ -323,12 +327,15 @@ class WeightedPriorityQueue :  public OpQueue <T, K>
       normal.insert(p, cl, cost, std::move(item), true);
     }
     T dequeue() override {
-      ceph_assert(strict.size + normal.size > 0);
+      ceph_assert(!empty());
       if (!strict.empty()) {
 	return strict.pop(true);
       }
       return normal.pop();
     }
+    unsigned get_size_slow() {
+      return strict.get_size_slow() + normal.get_size_slow();
+    }
     void dump(ceph::Formatter *f) const override {
       f->open_array_section("high_queues");
       strict.dump(f);
diff --git a/src/common/mClockPriorityQueue.h b/src/common/mClockPriorityQueue.h
index 5a87de35f51..9b96d5010c4 100644
--- a/src/common/mClockPriorityQueue.h
+++ b/src/common/mClockPriorityQueue.h
@@ -42,9 +42,8 @@ namespace ceph {
 
     typedef std::list<std::pair<cost_t, T> > ListPairs;
 
-    static unsigned filter_list_pairs(ListPairs *l,
-				      std::function<bool (T&&)> f) {
-      unsigned ret = 0;
+    static void filter_list_pairs(ListPairs *l,
+				  std::function<bool (T&&)> f) {
       for (typename ListPairs::iterator i = l->end();
 	   i != l->begin();
 	   /* no inc */
@@ -52,13 +51,11 @@ namespace ceph {
 	auto next = i;
 	--next;
 	if (f(std::move(next->second))) {
-	  ++ret;
 	  l->erase(next);
 	} else {
 	  i = next;
 	}
       }
-      return ret;
     }
 
     struct SubQueue {
@@ -68,7 +65,6 @@ namespace ceph {
       Classes q;
 
       unsigned tokens, max_tokens;
-      int64_t size;
 
       typename Classes::iterator cur;
 
@@ -78,13 +74,12 @@ namespace ceph {
 	: q(other.q),
 	  tokens(other.tokens),
 	  max_tokens(other.max_tokens),
-	  size(other.size),
 	  cur(q.begin()) {}
 
       SubQueue()
 	: tokens(0),
 	  max_tokens(0),
-	  size(0), cur(q.begin()) {}
+	  cur(q.begin()) {}
 
       void set_max_tokens(unsigned mt) {
 	max_tokens = mt;
@@ -117,14 +112,12 @@ namespace ceph {
 	q[cl].emplace_back(cost, std::move(item));
 	if (cur == q.end())
 	  cur = q.begin();
-	size++;
       }
 
       void enqueue_front(K cl, cost_t cost, T&& item) {
 	q[cl].emplace_front(cost, std::move(item));
 	if (cur == q.end())
 	  cur = q.begin();
-	size++;
       }
 
       const std::pair<cost_t, T>& front() const {
@@ -153,12 +146,14 @@ namespace ceph {
 	if (cur == q.end()) {
 	  cur = q.begin();
 	}
-	size--;
       }
 
-      unsigned length() const {
-	ceph_assert(size >= 0);
-	return (unsigned)size;
+      unsigned get_size_slow() const {
+	unsigned count = 0;
+	for (const auto& cls : q) {
+	  count += cls.second.size();
+	}
+	return count;
       }
 
       bool empty() const {
@@ -169,7 +164,7 @@ namespace ceph {
 	for (typename Classes::iterator i = q.begin();
 	     i != q.end();
 	     /* no-inc */) {
-	  size -= filter_list_pairs(&(i->second), f);
+	  filter_list_pairs(&(i->second), f);
 	  if (i->second.empty()) {
 	    if (cur == i) {
 	      ++cur;
@@ -187,7 +182,6 @@ namespace ceph {
 	if (i == q.end()) {
 	  return;
 	}
-	size -= i->second.size();
 	if (i == cur) {
 	  ++cur;
 	}
@@ -201,7 +195,7 @@ namespace ceph {
       }
 
       void dump(ceph::Formatter *f) const {
-	f->dump_int("size", size);
+	f->dump_int("size", get_size_slow());
 	f->dump_int("num_keys", q.size());
       }
     };
@@ -229,13 +223,13 @@ namespace ceph {
       // empty
     }
 
-    unsigned length() const override final {
+    unsigned get_size_slow() const {
       unsigned total = 0;
       total += queue_front.size();
       total += queue.request_count();
       for (auto i = high_queue.cbegin(); i != high_queue.cend(); ++i) {
-	ceph_assert(i->second.length());
-	total += i->second.length();
+	ceph_assert(i->second.get_size_slow());
+	total += i->second.get_size_slow();
       }
       return total;
     }
diff --git a/src/osd/mClockClientQueue.h b/src/osd/mClockClientQueue.h
index b8cfa0cb757..84454ff6bd0 100644
--- a/src/osd/mClockClientQueue.h
+++ b/src/osd/mClockClientQueue.h
@@ -53,8 +53,8 @@ namespace ceph {
 
     const crimson::dmclock::ClientInfo* op_class_client_info_f(const InnerClient& client);
 
-    inline unsigned length() const override final {
-      return queue.length();
+    inline unsigned get_size_slow() const {
+      return queue.get_size_slow();
     }
 
     // Ops of this priority should be deleted immediately
diff --git a/src/osd/mClockOpClassQueue.h b/src/osd/mClockOpClassQueue.h
index 30074079c80..f7d59283624 100644
--- a/src/osd/mClockOpClassQueue.h
+++ b/src/osd/mClockOpClassQueue.h
@@ -52,8 +52,8 @@ namespace ceph {
     const crimson::dmclock::ClientInfo*
     op_class_client_info_f(const osd_op_type_t& op_type);
 
-    inline unsigned length() const override final {
-      return queue.length();
+    inline unsigned get_size_slow() const {
+      return queue.get_size_slow();
     }
 
     // Ops of this priority should be deleted immediately
diff --git a/src/test/common/test_mclock_priority_queue.cc b/src/test/common/test_mclock_priority_queue.cc
index 4cdcaace4ce..8e8bcdf38cf 100644
--- a/src/test/common/test_mclock_priority_queue.cc
+++ b/src/test/common/test_mclock_priority_queue.cc
@@ -66,7 +66,7 @@ TEST(mClockPriorityQueue, Sizes)
   ceph::mClockQueue<Request,Client> q(&client_info_func);
 
   ASSERT_TRUE(q.empty());
-  ASSERT_EQ(0u, q.length());
+  ASSERT_EQ(0u, q.get_size_slow());
 
   Client c1(1);
   Client c2(2);
@@ -79,7 +79,7 @@ TEST(mClockPriorityQueue, Sizes)
   q.enqueue_strict(c2, 1, Request(6));
 
   ASSERT_FALSE(q.empty());
-  ASSERT_EQ(6u, q.length());
+  ASSERT_EQ(6u, q.get_size_slow());
 
 
   for (int i = 0; i < 6; ++i) {
@@ -87,7 +87,7 @@ TEST(mClockPriorityQueue, Sizes)
   }
 
   ASSERT_TRUE(q.empty());
-  ASSERT_EQ(0u, q.length());
+  ASSERT_EQ(0u, q.get_size_slow());
 }
 
 
@@ -260,11 +260,11 @@ TEST(mClockPriorityQueue, RemoveByClass)
     out.pop_front();
   }
 
-  ASSERT_EQ(6u, q.length()) << "after removal of three from client c2";
+  ASSERT_EQ(6u, q.get_size_slow()) << "after removal of three from client c2";
 
   q.remove_by_class(c3);
 
-  ASSERT_EQ(3u, q.length()) << "after removal of three from client c3";
+  ASSERT_EQ(3u, q.get_size_slow()) << "after removal of three from client c3";
   while (!q.empty()) {
     Request r = q.dequeue();
     ASSERT_TRUE((r.value & in_mask) > 0) <<
@@ -310,7 +310,7 @@ TEST(mClockPriorityQueue, RemoveByFilter)
     filtered.pop_front();
   }
 
-  ASSERT_EQ(5u, q.length()) <<
+  ASSERT_EQ(5u, q.get_size_slow()) <<
     "filter should have left five remaining elements";
   while (!q.empty()) {
     Request r = q.dequeue();
diff --git a/src/test/common/test_weighted_priority_queue.cc b/src/test/common/test_weighted_priority_queue.cc
index 9bb87177147..263fc4cb4d6 100644
--- a/src/test/common/test_weighted_priority_queue.cc
+++ b/src/test/common/test_weighted_priority_queue.cc
@@ -163,30 +163,30 @@ protected:
 TEST_F(WeightedPriorityQueueTest, wpq_size){
   WQ wq(0, 0);
   EXPECT_TRUE(wq.empty());
-  EXPECT_EQ(0u, wq.length());
+  EXPECT_EQ(0u, wq.get_size_slow());
 
   // Test the strict queue size.
   for (unsigned i = 1; i < 5; ++i) {
     wq.enqueue_strict(Klass(i),i, std::make_tuple(i, i, i));
     EXPECT_FALSE(wq.empty());
-    EXPECT_EQ(i, wq.length());
+    EXPECT_EQ(i, wq.get_size_slow());
   }
   // Test the normal queue size.
   for (unsigned i = 5; i < 10; ++i) {
     wq.enqueue(Klass(i), i, i, std::make_tuple(i, i, i));
     EXPECT_FALSE(wq.empty());
-    EXPECT_EQ(i, wq.length());
+    EXPECT_EQ(i, wq.get_size_slow());
   }
   // Test that as both queues are emptied
   // the size is correct.
   for (unsigned i = 8; i >0; --i) {
     wq.dequeue();
     EXPECT_FALSE(wq.empty());
-    EXPECT_EQ(i, wq.length());
+    EXPECT_EQ(i, wq.get_size_slow());
   }
   wq.dequeue();
   EXPECT_TRUE(wq.empty());
-  EXPECT_EQ(0u, wq.length());
+  EXPECT_EQ(0u, wq.get_size_slow());
 }
 
 TEST_F(WeightedPriorityQueueTest, wpq_test_static) {
@@ -228,7 +228,7 @@ TEST_F(WeightedPriorityQueueTest, wpq_test_remove_by_class) {
   wq.remove_by_class(k, &wq_removed);
   // Check that the right ops were removed.
   EXPECT_EQ(num_to_remove, wq_removed.size());
-  EXPECT_EQ(num_items - num_to_remove, wq.length());
+  EXPECT_EQ(num_items - num_to_remove, wq.get_size_slow());
   for (Removed::iterator it = wq_removed.begin();
        it != wq_removed.end(); ++it) {
     EXPECT_EQ(k, std::get<1>(*it));
diff --git a/src/test/osd/TestMClockClientQueue.cc b/src/test/osd/TestMClockClientQueue.cc
index 7fb6aabe27e..70e054c7b93 100644
--- a/src/test/osd/TestMClockClientQueue.cc
+++ b/src/test/osd/TestMClockClientQueue.cc
@@ -64,7 +64,7 @@ public:
 
 TEST_F(MClockClientQueueTest, TestSize) {
   ASSERT_TRUE(q.empty());
-  ASSERT_EQ(0u, q.length());
+  ASSERT_EQ(0u, q.get_size_slow());
 
   q.enqueue(client1, 12, 1u, create_snaptrim(100, client1));
   q.enqueue_strict(client2, 12, create_snaptrim(101, client2));
@@ -73,7 +73,7 @@ TEST_F(MClockClientQueueTest, TestSize) {
   q.enqueue(client1, 12, 1u, create_snaptrim(104, client1));
 
   ASSERT_FALSE(q.empty());
-  ASSERT_EQ(5u, q.length());
+  ASSERT_EQ(5u, q.get_size_slow());
 
   std::list<Request> reqs;
 
@@ -82,7 +82,7 @@ TEST_F(MClockClientQueueTest, TestSize) {
   reqs.push_back(q.dequeue());
 
   ASSERT_FALSE(q.empty());
-  ASSERT_EQ(2u, q.length());
+  ASSERT_EQ(2u, q.get_size_slow());
 
   q.enqueue_front(client2, 12, 1u, std::move(reqs.back()));
   reqs.pop_back();
@@ -94,14 +94,14 @@ TEST_F(MClockClientQueueTest, TestSize) {
   reqs.pop_back();
 
   ASSERT_FALSE(q.empty());
-  ASSERT_EQ(5u, q.length());
+  ASSERT_EQ(5u, q.get_size_slow());
 
   for (int i = 0; i < 5; ++i) {
     (void) q.dequeue();
   }
 
   ASSERT_TRUE(q.empty());
-  ASSERT_EQ(0u, q.length());
+  ASSERT_EQ(0u, q.get_size_slow());
 }
 
 
@@ -172,7 +172,7 @@ TEST_F(MClockClientQueueTest, TestRemoveByClass) {
     filtered_out.pop_front();
   }
 
-  ASSERT_EQ(3u, q.length());
+  ASSERT_EQ(3u, q.get_size_slow());
   Request r = q.dequeue();
   ASSERT_EQ(103u, r.get_map_epoch());
 
diff --git a/src/test/osd/TestMClockOpClassQueue.cc b/src/test/osd/TestMClockOpClassQueue.cc
index 1c9329cfdfa..932066979ba 100644
--- a/src/test/osd/TestMClockOpClassQueue.cc
+++ b/src/test/osd/TestMClockOpClassQueue.cc
@@ -66,7 +66,7 @@ public:
 
 TEST_F(MClockOpClassQueueTest, TestSize) {
   ASSERT_TRUE(q.empty());
-  ASSERT_EQ(0u, q.length());
+  ASSERT_EQ(0u, q.get_size_slow());
 
   q.enqueue(client1, 12, 0, create_snaptrim(100, client1));
   q.enqueue_strict(client2, 12, create_snaptrim(101, client2));
@@ -75,7 +75,7 @@ TEST_F(MClockOpClassQueueTest, TestSize) {
   q.enqueue(client1, 12, 0, create_snaptrim(104, client1));
 
   ASSERT_FALSE(q.empty());
-  ASSERT_EQ(5u, q.length());
+  ASSERT_EQ(5u, q.get_size_slow());
 
   std::list<Request> reqs;
 
@@ -84,7 +84,7 @@ TEST_F(MClockOpClassQueueTest, TestSize) {
   reqs.push_back(q.dequeue());
 
   ASSERT_FALSE(q.empty());
-  ASSERT_EQ(2u, q.length());
+  ASSERT_EQ(2u, q.get_size_slow());
 
   q.enqueue_front(client2, 12, 0, std::move(reqs.back()));
   reqs.pop_back();
@@ -96,14 +96,14 @@ TEST_F(MClockOpClassQueueTest, TestSize) {
   reqs.pop_back();
 
   ASSERT_FALSE(q.empty());
-  ASSERT_EQ(5u, q.length());
+  ASSERT_EQ(5u, q.get_size_slow());
 
   for (int i = 0; i < 5; ++i) {
     (void) q.dequeue();
   }
 
   ASSERT_TRUE(q.empty());
-  ASSERT_EQ(0u, q.length());
+  ASSERT_EQ(0u, q.get_size_slow());
 }
 
 
@@ -172,7 +172,7 @@ TEST_F(MClockOpClassQueueTest, TestRemoveByClass) {
     filtered_out.pop_front();
   }
 
-  ASSERT_EQ(3u, q.length());
+  ASSERT_EQ(3u, q.get_size_slow());
   Request r = q.dequeue();
   ASSERT_EQ(103u, r.get_map_epoch());