On 15/04/2016 12:31 a.m., Amos Jeffries wrote:
> This patch replaces the remaining use of Squid custom link_list type
> with STL std::queue or std::list templates. Removing the now unneeded
> custom type completely.
>
> It builds on the previous libmem old_api cleanup patch and has yet to be
> run tested, though the unit tests we have for the types all pass.
>
Testing on real traffic found a few bugs, which are fixed in the
attached patch. Though that means the cache unit tests do not actually
cover I/O to a file using store API, which is a fairly critical part of
cache testing IMO.
Amos
=== modified file 'src/Makefile.am'
--- src/Makefile.am 2016-03-18 09:38:10 +0000
+++ src/Makefile.am 2016-04-22 11:43:54 +0000
@@ -357,8 +357,6 @@
ipcache.cc \
ipcache.h \
$(LEAKFINDERSOURCE) \
- SquidList.h \
- SquidList.cc \
LogTags.cc \
LogTags.h \
lookup_t.h \
@@ -1058,8 +1056,6 @@
MasterXaction.h \
Notes.cc \
Notes.h \
- SquidList.h \
- SquidList.cc \
mem_node.cc \
Parsing.cc \
tests/stub_libsecurity.cc \
@@ -1297,8 +1293,6 @@
internal.cc \
LogTags.cc \
tests/stub_libsecurity.cc \
- SquidList.h \
- SquidList.cc \
MasterXaction.cc \
MasterXaction.h \
multicast.h \
@@ -1479,8 +1473,6 @@
HttpReply.cc \
int.h \
int.cc \
- SquidList.h \
- SquidList.cc \
MasterXaction.cc \
MasterXaction.h \
MemBuf.cc \
@@ -1725,8 +1717,6 @@
internal.cc \
LogTags.cc \
tests/stub_libsecurity.cc \
- SquidList.h \
- SquidList.cc \
MasterXaction.cc \
MasterXaction.h \
tests/stub_libmem.cc \
@@ -1964,8 +1954,6 @@
internal.h \
internal.cc \
LogTags.cc \
- SquidList.h \
- SquidList.cc \
MasterXaction.cc \
MasterXaction.h \
MemBuf.cc \
@@ -2199,8 +2187,6 @@
$(IPC_SOURCE) \
ipcache.cc \
LogTags.cc \
- SquidList.h \
- SquidList.cc \
MasterXaction.cc \
MasterXaction.h \
MemBuf.cc \
@@ -2516,8 +2502,6 @@
internal.cc \
LogTags.cc \
tests/stub_libsecurity.cc \
- SquidList.h \
- SquidList.cc \
MasterXaction.cc \
MasterXaction.h \
multicast.h \
@@ -2730,8 +2714,6 @@
RequestFlags.h \
int.h \
int.cc \
- SquidList.h \
- SquidList.cc \
MasterXaction.cc \
MasterXaction.h \
mem_node.cc \
@@ -2956,8 +2938,6 @@
int.cc \
RequestFlags.h \
RequestFlags.cc \
- SquidList.h \
- SquidList.cc \
Transients.cc \
MasterXaction.cc \
MasterXaction.h \
@@ -3138,8 +3118,6 @@
HttpReply.cc \
int.h \
int.cc \
- SquidList.h \
- SquidList.cc \
MasterXaction.cc \
MasterXaction.h \
MemBuf.cc \
@@ -3351,8 +3329,6 @@
internal.cc \
tests/stub_libeui.cc \
LogTags.cc \
- SquidList.h \
- SquidList.cc \
MasterXaction.cc \
MasterXaction.h \
multicast.h \
=== removed file 'src/SquidList.cc'
--- src/SquidList.cc 2016-01-01 00:12:18 +0000
+++ src/SquidList.cc 1970-01-01 00:00:00 +0000
@@ -1,49 +0,0 @@
-/*
- * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
- *
- * Squid software is distributed under GPLv2+ license and includes
- * contributions from numerous individuals and organizations.
- * Please see the COPYING and CONTRIBUTORS files for details.
- */
-
-/* DEBUG: none Linked list functions (deprecated) */
-
-#include "squid.h"
-#include "mem/forward.h"
-#include "SquidList.h"
-
-/* This should go away, in favour of the List template class */
-
-void
-linklistPush(link_list ** L, void *p)
-{
- link_list *l = (link_list *)memAllocate(MEM_LINK_LIST);
- l->next = NULL;
- l->ptr = p;
-
- while (*L)
- L = &(*L)->next;
-
- *L = l;
-}
-
-void *
-linklistShift(link_list ** L)
-{
- void *p;
- link_list *l;
-
- if (NULL == *L)
- return NULL;
-
- l = *L;
-
- p = l->ptr;
-
- *L = (*L)->next;
-
- memFree(l, MEM_LINK_LIST);
-
- return p;
-}
-
=== removed file 'src/SquidList.h'
--- src/SquidList.h 2016-01-01 00:12:18 +0000
+++ src/SquidList.h 1970-01-01 00:00:00 +0000
@@ -1,25 +0,0 @@
-/*
- * Copyright (C) 1996-2016 The Squid Software Foundation and contributors
- *
- * Squid software is distributed under GPLv2+ license and includes
- * contributions from numerous individuals and organizations.
- * Please see the COPYING and CONTRIBUTORS files for details.
- */
-
-/* DEBUG: none Linked list functions (deprecated) */
-
-#ifndef SQUID_SQUIDLIST_H_
-#define SQUID_SQUIDLIST_H_
-
-class link_list
-{
-public:
- void *ptr;
- link_list *next;
-};
-
-void linklistPush(link_list **, void *);
-void *linklistShift(link_list **);
-
-#endif /* SQUID_SQUIDLIST_H_ */
-
=== modified file 'src/fs/ufs/UFSStoreState.cc'
--- src/fs/ufs/UFSStoreState.cc 2016-01-01 00:12:18 +0000
+++ src/fs/ufs/UFSStoreState.cc 2016-04-22 18:08:03 +0000
@@ -14,7 +14,6 @@
#include "DiskIO/ReadRequest.h"
#include "DiskIO/WriteRequest.h"
#include "Generic.h"
-#include "SquidList.h"
#include "Store.h"
#include "store/Disk.h"
#include "UFSStoreState.h"
@@ -181,17 +180,18 @@
void
Fs::Ufs::UFSStoreState::doWrite()
{
- debugs(79, 3, HERE << this << " UFSStoreState::doWrite");
-
+ debugs(79, 3, static_cast<void *>(this));
assert(theFile->canWrite());
- _queued_write *q = (_queued_write *)linklistShift(&pending_writes);
-
- if (q == NULL) {
- debugs(79, 3, HERE << this << " UFSStoreState::doWrite queue is
empty");
+ if (pending_writes.empty()) {
+ debugs(79, 3, static_cast<void *>(this) << " queue is empty");
return;
}
+ auto *q = pending_writes.front();
+ assert(q);
+ pending_writes.pop();
+
if (theFile->error()) {
debugs(79, DBG_IMPORTANT,HERE << "avoid write on theFile with error");
debugs(79,3,HERE << "calling free_func for " << (void*) q->buf);
@@ -216,7 +216,7 @@
* coming in. For now let's just not use the writing flag at
* all.
*/
- debugs(79, 3, HERE << this << " calling theFile->write(" << q->size <<
")");
+ debugs(79, 3, static_cast<void *>(this) << " calling theFile->write(" <<
q->size << ")");
theFile->write(new WriteRequest(q->buf, q->offset, q->size, q->free_func));
delete q;
@@ -328,8 +328,6 @@
closing(false),
reading(false),
writing(false),
- pending_reads(NULL),
- pending_writes(NULL),
read_buf(NULL)
{
// StoreIOState inherited members
@@ -344,71 +342,67 @@
Fs::Ufs::UFSStoreState::~UFSStoreState()
{
- assert(pending_reads == NULL);
- assert(pending_writes == NULL);
+ assert(pending_reads.empty());
+ assert(pending_writes.empty());
}
void
Fs::Ufs::UFSStoreState::freePending()
{
- _queued_read *qr;
-
- while ((qr = (_queued_read *)linklistShift(&pending_reads))) {
- cbdataReferenceDone(qr->callback_data);
+ while (!pending_reads.empty()) {
+ auto *qr = pending_reads.front();
+ pending_reads.pop();
delete qr;
}
- debugs(79,3,HERE << "UFSStoreState::freePending: freed pending reads");
-
- _queued_write *qw;
-
- while ((qw = (_queued_write *)linklistShift(&pending_writes))) {
+ debugs(79, 3, "freed pending reads");
+
+ while (!pending_writes.empty()) {
+ auto *qw = pending_writes.front();
if (qw->free_func)
qw->free_func(const_cast<char *>(qw->buf));
+ pending_writes.pop();
delete qw;
}
- debugs(79,3,HERE << "UFSStoreState::freePending: freed pending writes");
+ debugs(79, 3, "freed pending writes");
}
bool
Fs::Ufs::UFSStoreState::kickReadQueue()
{
- _queued_read *q = (_queued_read *)linklistShift(&pending_reads);
-
- if (NULL == q)
- return false;
-
- debugs(79, 3, "UFSStoreState::kickReadQueue: reading queued request of "
<< q->size << " bytes");
+ if (pending_reads.empty())
+ return false;
+
+ auto *q = pending_reads.front();
+ if (!q)
+ return false;
+
+ debugs(79, 3, "reading queued request of " << q->size << " bytes");
+
+ pending_reads.pop();
void *cbdata;
-
if (cbdataReferenceValidDone(q->callback_data, &cbdata)) {
read_(q->buf, q->size, q->offset, q->callback, cbdata);
} else {
- debugs(79, 2, "UFSStoreState::kickReadQueue: this: " << this << "
cbdataReferenceValidDone returned false." << " closing: " << closing << "
flags.try_closing: " << flags.try_closing);
+ debugs(79, 2, "this: " << this << " cbdataReferenceValidDone returned
false. closing: " << closing << " flags.try_closing: " << flags.try_closing);
delete q;
return false;
}
delete q;
-
return true;
}
void
Fs::Ufs::UFSStoreState::queueRead(char *buf, size_t size, off_t aOffset, STRCB
*callback_, void *callback_data_)
{
- debugs(79, 3, "UFSStoreState::queueRead: queueing read");
+ debugs(79, 3, "queueing read");
assert(opening);
- assert (pending_reads == NULL);
- _queued_read *q = new _queued_read;
- q->buf = buf;
- q->size = size;
- q->offset = aOffset;
- q->callback = callback_;
- q->callback_data = cbdataReference(callback_data_);
- linklistPush(&pending_reads, q);
+ assert(pending_reads.empty());
+ auto *q = new QueuedRead(buf, size, aOffset, callback_, callback_data_);
+ pending_reads.push(q);
}
/*
@@ -435,7 +429,7 @@
flags.write_draining = true;
- while (pending_writes != NULL) {
+ while (!pending_writes.empty()) {
doWrite();
}
@@ -475,14 +469,9 @@
void
Fs::Ufs::UFSStoreState::queueWrite(char const *buf, size_t size, off_t
aOffset, FREE * free_func)
{
- debugs(79, 3, HERE << this << " UFSStoreState::queueWrite: queueing write
of size " << size);
+ debugs(79, 3, static_cast<void*>(this) << ": queueing write of size " <<
size);
- _queued_write *q;
- q = new _queued_write;
- q->buf = buf;
- q->size = size;
- q->offset = aOffset;
- q->free_func = free_func;
- linklistPush(&pending_writes, q);
+ auto *q = new QueuedWrite(buf, size, aOffset, free_func);
+ pending_writes.push(q);
}
=== modified file 'src/fs/ufs/UFSStoreState.h'
--- src/fs/ufs/UFSStoreState.h 2016-01-01 00:12:18 +0000
+++ src/fs/ufs/UFSStoreState.h 2016-04-22 18:16:24 +0000
@@ -10,9 +10,10 @@
#define SQUID_FS_UFS_UFSSTORESTATE_H
#include "DiskIO/IORequestor.h"
-#include "SquidList.h"
#include "StoreIOState.h"
+#include <queue>
+
namespace Fs
{
namespace Ufs
@@ -44,17 +45,18 @@
protected:
virtual void doCloseCallback (int errflag);
- class _queued_read
+ class QueuedRead
{
- MEMPROXY_CLASS(UFSStoreState::_queued_read);
+ MEMPROXY_CLASS(UFSStoreState::QueuedRead);
public:
- _queued_read() :
- buf(nullptr),
- size(0),
- offset(0),
- callback(nullptr),
- callback_data(nullptr)
+ QueuedRead(char *aBuf, size_t bufSz, off_t theOffset, STRCB *cb, void
*data) :
+ buf(aBuf),
+ size(bufSz),
+ offset(theOffset),
+ callback(cb),
+ callback_data(cbdataReference(data))
{}
+ ~QueuedRead() {cbdataReferenceDone(callback_data);}
char *buf;
size_t size;
@@ -63,15 +65,15 @@
void *callback_data;
};
- class _queued_write
+ class QueuedWrite
{
- MEMPROXY_CLASS(UFSStoreState::_queued_write);
+ MEMPROXY_CLASS(UFSStoreState::QueuedWrite);
public:
- _queued_write() :
- buf(nullptr),
- size(0),
- offset(0),
- free_func(nullptr)
+ QueuedWrite(const char *aBuf, size_t bufSz, off_t theOffset, FREE
*func) :
+ buf(aBuf),
+ size(bufSz),
+ offset(theOffset),
+ free_func(func)
{}
char const *buf;
@@ -98,8 +100,8 @@
*/
bool try_closing;
} flags;
- link_list *pending_reads;
- link_list *pending_writes;
+ std::queue<QueuedRead *> pending_reads;
+ std::queue<QueuedWrite *> pending_writes;
void queueRead(char *, size_t, off_t, STRCB *, void *);
void queueWrite(char const *, size_t, off_t, FREE *);
bool kickReadQueue();
=== modified file 'src/mem/forward.h'
--- src/mem/forward.h 2016-04-22 11:39:23 +0000
+++ src/mem/forward.h 2016-04-23 08:36:03 +0000
@@ -46,7 +46,6 @@
MEM_64K_BUF,
MEM_ACL_DENY_INFO_LIST,
MEM_ACL_NAME_LIST,
- MEM_LINK_LIST,
MEM_DLINK_NODE,
MEM_DREAD_CTRL,
MEM_DWRITE_Q,
=== modified file 'src/mem/old_api.cc'
--- src/mem/old_api.cc 2016-04-22 11:39:23 +0000
+++ src/mem/old_api.cc 2016-04-23 08:36:03 +0000
@@ -24,7 +24,6 @@
#include "MemBuf.h"
#include "mgr/Registration.h"
#include "SquidConfig.h"
-#include "SquidList.h"
#include "SquidTime.h"
#include "Store.h"
@@ -445,7 +444,6 @@
memDataInit(MEM_ACL_DENY_INFO_LIST, "AclDenyInfoList",
sizeof(AclDenyInfoList), 0);
memDataInit(MEM_ACL_NAME_LIST, "acl_name_list", sizeof(AclNameList), 0);
- memDataInit(MEM_LINK_LIST, "link_list", sizeof(link_list), 10);
memDataInit(MEM_DLINK_NODE, "dlink_node", sizeof(dlink_node), 10);
memDataInit(MEM_DREAD_CTRL, "dread_ctrl", sizeof(dread_ctrl), 0);
memDataInit(MEM_DWRITE_Q, "dwrite_q", sizeof(dwrite_q), 0);
=== modified file 'src/repl/heap/store_repl_heap.cc'
--- src/repl/heap/store_repl_heap.cc 2016-01-01 00:12:18 +0000
+++ src/repl/heap/store_repl_heap.cc 2016-04-22 18:20:50 +0000
@@ -20,7 +20,6 @@
#include "squid.h"
#include "heap.h"
#include "MemObject.h"
-#include "SquidList.h"
#include "Store.h"
#include "store_heap_replacement.h"
#include "wordlist.h"
@@ -181,10 +180,12 @@
/** RemovalPurgeWalker **/
-typedef struct _HeapPurgeData HeapPurgeData;
+class HeapPurgeData
+{
+public:
+ HeapPurgeData() : min_age(0.0) {}
-struct _HeapPurgeData {
- link_list *locked_entries;
+ std::list<StoreEntry *> locked_entries;
heap_key min_age;
};
@@ -197,22 +198,18 @@
StoreEntry *entry;
heap_key age;
-try_again:
-
- if (heap_empty(h->theHeap))
- return NULL; /* done */
-
- age = heap_peepminkey(h->theHeap);
-
- entry = (StoreEntry *)heap_extractmin(h->theHeap);
-
- if (entry->locked()) {
-
- entry->lock("heap_purgeNext");
- linklistPush(&heap_walker->locked_entries, entry);
-
- goto try_again;
- }
+ do {
+ if (heap_empty(h->theHeap))
+ return nullptr;
+
+ age = heap_peepminkey(h->theHeap);
+ entry = (StoreEntry *)heap_extractmin(h->theHeap);
+
+ if (entry->locked()) {
+ entry->lock("heap_purgeNext");
+ heap_walker->locked_entries.push_back(entry);
+ }
+ } while (entry->locked());
heap_walker->min_age = age;
h->setPolicyNode(entry, NULL);
@@ -225,7 +222,6 @@
HeapPurgeData *heap_walker = (HeapPurgeData *)walker->_data;
RemovalPolicy *policy = walker->_policy;
HeapPolicyData *h = (HeapPolicyData *)policy->_data;
- StoreEntry *entry;
assert(strcmp(policy->_type, "heap") == 0);
assert(h->nwalkers > 0);
h->nwalkers -= 1;
@@ -235,13 +231,13 @@
debugs(81, 3, "Heap age set to " << h->theHeap->age);
}
- /*
- * Reinsert the locked entries
- */
- while ((entry = (StoreEntry
*)linklistShift(&heap_walker->locked_entries))) {
+ // Reinsert the locked entries
+ while (!heap_walker->locked_entries.empty()) {
+ auto *entry = heap_walker->locked_entries.front();
heap_node *node = heap_insert(h->theHeap, entry);
h->setPolicyNode(entry, node);
entry->unlock("heap_purgeDone");
+ heap_walker->locked_entries.pop_front();
}
safe_free(walker->_data);
@@ -253,14 +249,10 @@
{
HeapPolicyData *h = (HeapPolicyData *)policy->_data;
RemovalPurgeWalker *walker;
- HeapPurgeData *heap_walk;
h->nwalkers += 1;
walker = new RemovalPurgeWalker;
- heap_walk = (HeapPurgeData *)xcalloc(1, sizeof(*heap_walk));
- heap_walk->min_age = 0.0;
- heap_walk->locked_entries = NULL;
walker->_policy = policy;
- walker->_data = heap_walk;
+ walker->_data = new HeapPurgeData;
walker->max_scan = max_scan;
walker->Next = heap_purgeNext;
walker->Done = heap_purgeDone;
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
