From 90fb604e24f30efdb7a26c3c09a02b7b08169f4b Mon Sep 17 00:00:00 2001 From: Meera Mosale Nataraja Date: Thu, 3 Dec 2015 15:31:00 -0800 Subject: [PATCH 1/3] TS-4050 - Fix trafficserver crash when buckets=0 is configured in cache_promote plugin and Set buckets default value to be 10 --- .../cache_promote/cache_promote.cc | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/plugins/experimental/cache_promote/cache_promote.cc b/plugins/experimental/cache_promote/cache_promote.cc index dab961d3420..67e04b7957e 100644 --- a/plugins/experimental/cache_promote/cache_promote.cc +++ b/plugins/experimental/cache_promote/cache_promote.cc @@ -32,6 +32,7 @@ #include "ts/remap.h" #include "ts/ink_config.h" +#define DEFAULT_BUCKET_SIZE 10 static const char *PLUGIN_NAME = "cache_promote"; TSCont gNocacheCont; @@ -138,8 +139,8 @@ class ChancePolicy : public PromotionPolicy ////////////////////////////////////////////////////////////////////////////////////////////// -// The LRU based policy keeps track of number of URLs, with a counter for each slot. -// Objects are not promoted unless the counter reaches before it gets evicted. An +// The LRU basedo policy keeps track of number of URLs, with a counter for each slot. +// Objects are not promted unless the counter reaches before it gets evicted. An // optional parameter can be used to sample hits, this can reduce contention and // churning in the LRU as well. // @@ -209,6 +210,12 @@ class LRUPolicy : public PromotionPolicy switch (opt) { case 'b': _buckets = static_cast(strtol(optarg, NULL, 10)); + if (_buckets <= 0) { + // buckets size of 0 doesn't make sense and is not supported. Set to default value of 10. + TSDebug(PLUGIN_NAME, "buckets size of 0 is not allowed. Use buckets size >= 10"); + TSDebug(PLUGIN_NAME, "Setting to default bucket size of 10"); + _buckets = DEFAULT_BUCKET_SIZE; + } break; case 'h': _hits = static_cast(strtol(optarg, NULL, 10)); @@ -252,13 +259,19 @@ class LRUPolicy : public PromotionPolicy if (++(map_it->second->second) >= _hits) { // Promoted! Cleanup the LRU, and signal success. Save the promoted entry on the freelist. TSDebug(PLUGIN_NAME, "saving the LRUEntry to the freelist"); - _freelist.splice(_freelist.begin(), _list, map_it->second); + // Check if list is not empty + if (!_list.empty()) { + _freelist.splice(_freelist.begin(), _list, map_it->second); + } _map.erase(map_it->first); ret = true; } else { // It's still not promoted, make sure it's moved to the front of the list TSDebug(PLUGIN_NAME, "still not promoted, got %d hits so far", map_it->second->second); - _list.splice(_list.begin(), _list, map_it->second); + // Check if list is not empty + if (!_list.empty()) { + _list.splice(_list.begin(), _list, map_it->second); + } } } else { // New LRU entry for the URL, try to repurpose the list entry as much as possible From 80e9c31de7d98d86fd484ed7e30ce13ecc189173 Mon Sep 17 00:00:00 2001 From: Meera Mosale Nataraja Date: Fri, 4 Dec 2015 14:30:12 -0800 Subject: [PATCH 2/3] Add assert statement as suggested in review comments --- plugins/experimental/cache_promote/cache_promote.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/plugins/experimental/cache_promote/cache_promote.cc b/plugins/experimental/cache_promote/cache_promote.cc index 67e04b7957e..a5a8a15728b 100644 --- a/plugins/experimental/cache_promote/cache_promote.cc +++ b/plugins/experimental/cache_promote/cache_promote.cc @@ -256,22 +256,17 @@ class LRUPolicy : public PromotionPolicy map_it = _map.find(&hash); if (_map.end() != map_it) { // We have an entry in the LRU + TSReleaseAssert(_list.size() > 0); if (++(map_it->second->second) >= _hits) { // Promoted! Cleanup the LRU, and signal success. Save the promoted entry on the freelist. TSDebug(PLUGIN_NAME, "saving the LRUEntry to the freelist"); - // Check if list is not empty - if (!_list.empty()) { - _freelist.splice(_freelist.begin(), _list, map_it->second); - } + _freelist.splice(_freelist.begin(), _list, map_it->second); _map.erase(map_it->first); ret = true; } else { // It's still not promoted, make sure it's moved to the front of the list TSDebug(PLUGIN_NAME, "still not promoted, got %d hits so far", map_it->second->second); - // Check if list is not empty - if (!_list.empty()) { - _list.splice(_list.begin(), _list, map_it->second); - } + _list.splice(_list.begin(), _list, map_it->second); } } else { // New LRU entry for the URL, try to repurpose the list entry as much as possible From f3c82a4a68de98acfd88bd441e9a178633b2fbb9 Mon Sep 17 00:00:00 2001 From: Meera Mosale Nataraja Date: Fri, 4 Dec 2015 14:35:52 -0800 Subject: [PATCH 3/3] Addressed review comments by adding TSReleaseAssert --- plugins/experimental/cache_promote/cache_promote.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/experimental/cache_promote/cache_promote.cc b/plugins/experimental/cache_promote/cache_promote.cc index a5a8a15728b..056aaaba8d4 100644 --- a/plugins/experimental/cache_promote/cache_promote.cc +++ b/plugins/experimental/cache_promote/cache_promote.cc @@ -256,7 +256,7 @@ class LRUPolicy : public PromotionPolicy map_it = _map.find(&hash); if (_map.end() != map_it) { // We have an entry in the LRU - TSReleaseAssert(_list.size() > 0); + TSReleaseAssert(_list.size() > 0); // mismatch in the LRUs hash and list if (++(map_it->second->second) >= _hits) { // Promoted! Cleanup the LRU, and signal success. Save the promoted entry on the freelist. TSDebug(PLUGIN_NAME, "saving the LRUEntry to the freelist");