On 2014-01-10 04:50, Tsantilas Christos wrote:
On 01/09/2014 12:07 AM, Amos Jeffries wrote:
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
Some notes before proceed to any change in the patch. I need squid
developers opinions.
If you compare the MemMap and StoreMap code you will see that they are
similar. The original goal was to keep the code similar in order to
merge these two classes to one in the future.
This is why some of the variable documentations does not make sense
for
this class, and this is why the sfileno is used instead of an unsigned
integer type.
Thank you for explaining sfileno. Its seems weird but is okay. A code
comment to explain would not go amiss.
As for the code comments describing weirdness, please make them correct
for the class as of *this* submission. They can (and should) be changed
during that future merge to be appropriate for design decisions made
during those changes which likely turn out to be different from what we
currently assume. Making them correct for the now helps avoid people
breaking the current API.
This is something I should mention when I posted the patch but this
patch implemented a long time ago, and I had forgot it. I just re-read
internal Measurement Factory mails about this. Sorry for this.
I am suggesting to keep MemMap as is (with only the FlexibleArrays
fixes) just to have it similar to StoreMap class.
In the future we should merge the Ipc::StoreMap and Ipc::MemMap to a
Ipc::SharedCache class. Then we can make more fixes here.
Opinions?
From the earlier description I was thinking you were intending to make
this MemMap a generic memory cache class in the sense that we could use
it for the non-Store caching Squid does (ie auth credentials, helper
results, DNS results) not just StoreMap HTTP objects. That is a base
class we could really use to fix the outstanding non-SMP components.
With that assumption it makes more sense to make StoreMap inherit from
this class and add the Store-specific bits on top. This class just doing
generic cache management details (key/ttl/add/remove/update) in
SMP-aware operations.
Given your plans, the request to separate SSL changes from MemMap
addition is even more important to be done so that future Store updates
do not result in someone inexperienced attempting to it themselves when
back-porting a future Store change.
Most of the requests are styling and documentation fixes. They do not
change with this extra detail, except the one about use of size_t can be
ignored.
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
