[squid-dev] RFC: Transitioning ipcache and fqdncache to ClpMap

[Alex Rousskov]

On 7/11/23 03:35, Francesco Chemolli wrote:

> I'd like to start working on transitioning ipcache and fqdncache to 
> ClpMap.

Thank you. Please _plan_ to convert them both (as you are already doing 
here!), but then convert them one at a time (to avoid duplicating 
review/modification efforts). I bet we will need at least 6 "minimal" 
PRs here.


> There is only one snag,

FWIW, I did not check all the details, but I see two more snags, 
including a very important/difficult one:

2. ipcache.cc code "locks" entries to prevent their removal from the 
cache in certain cases. See ipcache_entry::locks. The corresponding code 
already has serious problems, but the approach itself needs to be 
refactored using RefCount, so that a cache entry can be removed from the 
cache at any time. This is not going to be easy and will require at 
least one dedicated PR, but I believe it is an essential preliminary step.

3. We should also remove ipcache_low and ipcache_high directives before 
the conversion. Those two parameters do not make sense for an 
instant-removal cache: They are not needed for anything useful, create 
performance anomalies, and increase code/configuration complexity. The 
corresponding logic is not supported by ClpMap.


Now back to your snag #1:

> the current configuration directives specify a cache 
> size in number of entries, where ClpMap specifies a max size.
> 
> I can see two ways forward:
> 1. Add a second max-size parameter to ClpMap, ensuring that it starts 
> expunging entries when the maximum capacity in either memory use or 
> number of entries is reached

I am not against this option, especially if we deprecate/remove existing 
count-based configuration (ipcache_size and friends) and add byte-based 
configuration to replace it (the directive name is to be determined).


> 2. guesstimate how many bytes of memory an ipcache/fqdncache uses, and 
> convert

I support this option if we deprecate/remove existing counter-based 
configuration (ipcache_size and friends) and add bytes-based 
configuration (the directive name is to be determined). I prefer this 
option in this case because it avoids making ClpMap code more complex 
long-term just to accommodate a short-term deprecation need. The extra 
code complexity is not huge, but cache limit enforcement is tricky, and 
the history of the related ClpMap code suggests that adding a new limit 
vector will take several (likely painful) iterations.


> Regardless, I believe that from the user-facing perspective, a solid way 
> forward is to give to the ipcache_size and fqdncache_size directives an 
> option to specify a max size in entries or, by adding a memory-size 
> suffix, in used memory, and possibly deprecate the max-size-in-entries 
> option in Squid 7 and retire it in squid 8.

In the vast majority of deployments, the existing count-based limit is 
inferior to the byte-based limit because memory bytes is a real resource 
that admins understand and have to optimize/ration/limit (while the 
number of entries has no direct relationship to anything most admins 
should care about). Thus, long-term, we should provide byte-based 
configuration options.

To keep things simple and to facilitate deprecation, I recommend adding 
two new directives instead of making the existing directives more 
complex. Naming is a separate/secondary issue, but we can model the new 
names based on the two existing primary caches (cache_dir and 
cache_mem).  Alternatively, we can use a more natural(?) word order. 
Here are a few variants (from most to least preferred by me):

* dns_cache and reverse_dns_cache
* cache_dns and cache_dns_reverse
* cache_ip and cache_domain


HTH,

Alex.