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

[Alex Rousskov]

[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.