Re: [PATCH] SMP SSL session cache implementation

On 2014-01-09 07:30, Tsantilas Christos wrote:
> This patch implement SSL session cache sharing across SMP workers using
> shared memory. The following new squid configuration options added:
>
> - The "sslproxy_session_cache_size" option which sets the cache size
> to
> use for ssl session. Example usage:
> sslproxy_session_cache_size 4 MB
>
> - The "sslproxy_session_ttl" option which defines the time in seconds
> the ssl session is valid. Example usage:
> sslproxy_session_ttl 600
>
> This patch also investigates the Ipc::MemMap class to help squid
> developers implement shared caches for squid processes.
>
> This is a Measurement Factory project

general issues:

* Please do not add HERE macro in new code.

* MemMap seems to be combining the concepts and operations of an indexed
cache with timeouts. "map"'s do not have the same semantics. Perhaps
calling it Ipc::SharedCache would be better.

* Please finalize and merge the MemMap feature separately so it is not
tied to the SSL changes.

in src/ipc/MemMap.cc:

* why is sfileno being used to references slots? It is a type
specifically designed for operations matching inode filesystem
semantics. It is dragging in size limitations which memory caches are
perhaps better off without.

* please implement the XXX notes about FlexibleArray
  - XXX also in .h

* +Ipc::MemMap::openForWriting()
  - "XXX: the caller should do that" already seems to be implemented by
commenting out the setKey(). Please remove the whole line if it is
working as-is right now.
  -

* Ipc::MemMap::abortIo() comment about caller is irrelevant and wrong
(caller could be anything). The only relevant thing is what state the
*slot* is in and the code explains that well enough.
  - Please remove that comment.

* entryCount() / entryLimit() and possibly valid() should be unsigned
type (size_t probably).

in src/ipc/MemMap.h:

* Please update class members order/layout to match Squid-3 coding
guidelines.
  - see
http://wiki.squid-cache.org/Squid3CodingGuidelines#Class_declaration_layout

* If MemMap is supposed to be used outside SSL why is it depending on
macros named SSL_SESSION_* ?
  - please rename those to something more generic.

* comment explaining class MemMap appears to have multiple sentences.
Please use upper case and full stops. The doxygen output will run those
lines into a single sentence if you dont write clearly.

* openForWriting() comment "finds, reservers space for " is not clear.
  - what exactly is is saying anyway?
  - there is also a missing empty line between this function declaration
and the next ones comment.

* comment "notified before a readable entry is freed" for cleaner.
Please clarify what cleaner is in relation to MemMap and how "notify"
happens (callback? require class method?).

* please use SBuf instead of String for new variables (eg SBuf path)

* Ipc::MemMapSlot::set( ... expireOn)
  - s/expireOn/expireAt/ - English weird grammar. time_t is treated as
absolute value so 'at' (as opposed to offset in a scale which would be
'on').

* How about moving waitingToBeFreed above the lock. That way the
comments are more consistent with what the lock controls.

* please update documentation of State::Writeable. "transitions from
Empty to Readable" does not explain anything unless one already knows
what the state means.
  - perhapse Writeable should be described "in process of being written
to, not Empty and not yet Readable". If so then it should also probably
be renamed "Writing" to match the active semantic.

in src/main.cc:

* the #if USE_SSL macro does not need adding twice in a row. Please
combine the USE_SSL operations under one wrapper #if...#endif

in src/ssl/support.cc:

* store_session_cb() and remove_session_cb() should be static yes?

* why is store_session_cb() always returning 0?
  - surely the absence of a session cache when the callback occurs should
be an error.
  - also the comment question should be marked with a TODO

* get_session_cb()
  - double-empty lines near the top of the function.

* Ssl::initialize_session_cache()
  - the for loops seem to be doing a lot of useless walking in the case
that "else SslSessionCache = NULL;". Please return immediately after
setting that =NULL value.

Amos
Received on Wed Jan 08 2014 - 22:07:48 MST