[squid-dev] Squid: Small packets and low performance between squid and icap

[Prashanth Prabhu]

[+ squid-dev; bcc ssquid-users]

Hi Alex,

Sorry about the late reply.

Please see inline.

>> Here's the behavior I have seen: When the connection is set up, the
>> buffer gets a size of 16KB (default). Squid reads from the socket,
>> parses the data, and then sends it towards c-icap as appropriate. Now,
>> as part of parsing the data, the buffer is NUL-terminated via a call
>> to c_str(). This NUL-termination, however, is not accounted for by an
>> increase in the "offset" (off) in the underlying MemBlob, therefore,
>> the offset and size go out of sync.
>
> Just to avoid a misunderstanding:
>
> * MemBlob does not have an "offset".

Indeed. I was imprecise in my explanation -- the effect of drafting
email even as I was in the middle of investigating the code.

The c_str() code doesn't increment SBuf::len_. As a result the
MemBlob::canAppend() call, which takes in SBuf::off_ and SBuf::len_
doesn't match the MemBlob::size_, which was incremented as part of the
c_str() call.


> * A call to c_str() should not increase SBuf::len_  either because it
> does not add a new character to the SBuf object. That call just
> terminates the underlying buffer.

Well, without an increment of the MemBlob::size_ (or with an increment
to the SBuf::len_) this would have been OK. However, once that 'size_'
is incremented, we have the SBuf concept of how much buffer is used
being "out of sync" with the MemBlob's conception of buffer usage.

FWIW I don't quite understand how the NUL-char doesn't add a new
character to the SBuf object. Yes, it is terminating the string, but
in 'C' it is also a legit character. So, unclear what we are
attempting with this magic; or why. Seems to me, not incrementing
'len_' is a mistake.


> Single-owner optimizations aside (a known TODO), the above is the
> desired behavior according to the documented c_str() guarantees:

Can you please explain or point me to a document that has more info
about this "Single-owner" optimization?

>
>>      * The returned value points to an internal location whose contents
>>      * are guaranteed to remain unchanged only until the next call
>>      * to a non-constant member function of the SBuf object.
>
> In other words, we cannot allow some _other_ SBuf object to overwrite
> our null-termination character in the MemBlob we share with that other SBuf.
>
> The high price for that strong guarantee is one of the reasons we should
> avoid c_str() calls in Squid code.

Note that the issue that I have explained is from a mostly stock squid
3.5.1 codebase. This isn't stemming from new c_str() calls added to
the codebase.


>> When canAppend() fails, a new
>> buffer is re-allocated. When this reallocation occurs, however, the
>> new size of the buffer is dependent on the size being reserved.
>
> If we are still talking about the I/O buffer (and not just some random
> SBuf string somewhere), then the I/O buffer _capacity_ should not shrink
> below a certain minimum, regardless of how much content the buffer has
> already stored. There should be some Squid code that ensures the minimum
> capacity of the I/O buffer used to read requests. If it is missing, it
> is a Squid bug.

It does shrink, as you can see from the debugs that I posted earlier.


>> As a temporary measure, I have an experimental change that checks
>> whether the body size is known and if known always reserves a large
>> enough size (currently 16K).
>
> It is difficult to discuss this without seeing your changes, but the
> reservation should probably be unconditional -- the I/O buffer capacity
> should always be at least 16KB (or whatever size we start with).

Yes, that would be another way of fixing this issue.

I have posted the current changes that are currently working for the
most part below. It uses current capacity size as a heuristic to bump
size up. The diff also has some previous fixes, that were pointed out
to me on the thread, embedded in it.

---
diff --git a/3rdparty/squid-3.5.1/src/MemBlob.h b/3rdparty/squid-3.5.1/src/Mem
Blob.h
index b96330e..d265576 100644
--- a/3rdparty/squid-3.5.1/src/MemBlob.h
+++ b/3rdparty/squid-3.5.1/src/MemBlob.h
@@ -94,6 +94,8 @@ public:
     /// extends the available space to the entire allocated blob
     void clear() { size = 0; }

+    size_type currentCapacity() const { return (capacity); };
+
     /// dump debugging information
     std::ostream & dump(std::ostream &os) const;

diff --git a/3rdparty/squid-3.5.1/src/SBuf.cc b/3rdparty/squid-3.5.1/src/SBuf.
cc
index 53221d6..91886a0 100644
--- a/3rdparty/squid-3.5.1/src/SBuf.cc
+++ b/3rdparty/squid-3.5.1/src/SBuf.cc
@@ -76,7 +76,7 @@ SBufStats::operator +=(const SBufStats& ss)
 SBuf::SBuf()
     : store_(GetStorePrototype()), off_(0), len_(0)
 {
-    debugs(24, 8, id << " created");
+    debugs(24, 8, id << " created, size=" << spaceSize());
     ++stats.alloc;
     ++stats.live;
 }
@@ -171,6 +171,7 @@ SBuf::rawSpace(size_type minSpace)
     // the store knows the last-used portion. If
     // it's available, we're effectively claiming ownership
     // of it. If it's not, we need to go away (realloc)
+    debugs(24, 7, "off_=" << off_ << "; len_=" << len_ << "; size="
<< store_->size);
     if (store_->canAppend(off_+len_, minSpace)) {
         debugs(24, 7, "not growing");
         return bufEnd();
diff --git a/3rdparty/squid-3.5.1/src/SBuf.h b/3rdparty/squid-3.5.1/src/SBuf.h
index ef77733..5bb3ef4 100644
--- a/3rdparty/squid-3.5.1/src/SBuf.h
+++ b/3rdparty/squid-3.5.1/src/SBuf.h
@@ -541,6 +541,8 @@ public:
     /// std::string export function
     std::string toStdString() const { return std::string(buf(),length()); }

+    size_type currentCapacity() const { return (store_->currentCapacity()); }
+
     // TODO: possibly implement erase() similar to std::string's erase
     // TODO: possibly implement a replace() call
 private:
diff --git a/3rdparty/squid-3.5.1/src/client_side.cc
b/3rdparty/squid-3.5.1/src/client_side.cc
index f2d0ce0..e191550 100644
--- a/3rdparty/squid-3.5.1/src/client_side.cc
+++ b/3rdparty/squid-3.5.1/src/client_side.cc
@@ -2347,6 +2348,9 @@ ConnStateData::In::maybeMakeSpaceAvailable()
             buf.reserveCapacity(wantCapacity);
         }
         debugs(33, 2, "growing request buffer: available=" <<
buf.spaceSize() << " used=" << buf.length());
+    } else if (buf.spaceSize() < buf.currentCapacity() / 2) {
+        debugs(33, 2, "growing request buffer: available=" <<
buf.spaceSize() << " used=" << buf.length());
+        buf.reserveCapacity(CLIENT_REQ_BUF_SZ * 4);
     }
     return (buf.spaceSize() >= 2);
 }
@@ -3244,6 +3248,7 @@ ConnStateData::clientReadRequest(const CommIoCbParams &io)
      * Plus, it breaks our lame *HalfClosed() detection
      */

+    in.maybeMakeSpaceAvailable();
     CommIoCbParams rd(this); // will be expanded with ReadNow results
     rd.conn = io.conn;
     switch (Comm::ReadNow(rd, in.buf)) {
@@ -3354,11 +3359,13 @@ ConnStateData::handleRequestBodyData()
             return false;
         }
     } else { // identity encoding
-        debugs(33,5, HERE << "handling plain request body for " <<
clientConnection);
-        putSize = bodyPipe->putMoreData(in.buf.c_str(), in.buf.length());
-        if (!bodyPipe->mayNeedMoreData()) {
-            // BodyPipe will clear us automagically when we produced everything
-            bodyPipe = NULL;
+        debugs(33,5, HERE << "handling plain request body for " <<
clientConnection << "; len=" << in.buf.length());
+        if (in.buf.length() > 0) {
+            putSize = bodyPipe->putMoreData(in.buf.c_str(), in.buf.length());
+            if (!bodyPipe->mayNeedMoreData()) {
+                // BodyPipe will clear us automagically when we
produced everything
+                bodyPipe = NULL;
+            }
         }
     }

@@ -3537,9 +3544,6 @@ ConnStateData::start()
     BodyProducer::start();
     HttpControlMsgSink::start();

-    // ensure a buffer is present for this connection
-    in.maybeMakeSpaceAvailable();
-
     if (port->disable_pmtu_discovery != DISABLE_PMTU_OFF &&
             (transparent() || port->disable_pmtu_discovery ==
DISABLE_PMTU_ALWAYS)) {
 #if defined(IP_MTU_DISCOVER) && defined(IP_PMTUDISC_DONT)
---


Regards.
Prashanth

On 9 February 2016 at 13:54, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> [this should be on squid-dev instead]
>
> On 02/09/2016 01:20 PM, Prashanth Prabhu wrote:
>
>> Here's the behavior I have seen: When the connection is set up, the
>> buffer gets a size of 16KB (default). Squid reads from the socket,
>> parses the data, and then sends it towards c-icap as appropriate. Now,
>> as part of parsing the data, the buffer is NUL-terminated via a call
>> to c_str(). This NUL-termination, however, is not accounted for by an
>> increase in the "offset" (off) in the underlying MemBlob, therefore,
>> the offset and size go out of sync.
>
> Just to avoid a misunderstanding:
>
> * MemBlob does not have an "offset".
>
> * SBuf::off_ should not change when we are adding characters to SBuf
> because it is the start of the buffer, not the end of it.
>
> * A call to c_str() should not increase SBuf::len_  either because it
> does not add a new character to the SBuf object. That call just
> terminates the underlying buffer.
>
> Based on your comments below, I think I know what you mean by "go out of
> sync", but everything is as "in sync" as it can be when one adds
> termination characters that are not really there from SBuf::length()
> point of view. The bug is elsewhere.
>
>
>> MemBlob::canAppend() failing because
>> MemBlob::isAppendOffset() fails -- the 'off' and 'size' are not the
>> same due to the above c_str() call.
>
> Single-owner optimizations aside (a known TODO), the above is the
> desired behavior according to the documented c_str() guarantees:
>
>>      * The returned value points to an internal location whose contents
>>      * are guaranteed to remain unchanged only until the next call
>>      * to a non-constant member function of the SBuf object.
>
> In other words, we cannot allow some _other_ SBuf object to overwrite
> our null-termination character in the MemBlob we share with that other SBuf.
>
> The high price for that strong guarantee is one of the reasons we should
> avoid c_str() calls in Squid code.
>
>
>> When canAppend() fails, a new
>> buffer is re-allocated. When this reallocation occurs, however, the
>> new size of the buffer is dependent on the size being reserved.
>
> If we are still talking about the I/O buffer (and not just some random
> SBuf string somewhere), then the I/O buffer _capacity_ should not shrink
> below a certain minimum, regardless of how much content the buffer has
> already stored. There should be some Squid code that ensures the minimum
> capacity of the I/O buffer used to read requests. If it is missing, it
> is a Squid bug.
>
>
>> As a temporary measure, I have an experimental change that checks
>> whether the body size is known and if known always reserves a large
>> enough size (currently 16K).
>
> It is difficult to discuss this without seeing your changes, but the
> reservation should probably be unconditional -- the I/O buffer capacity
> should always be at least 16KB (or whatever size we start with).
>
>
> HTH,
>
> Alex.
>