maskit commented on a change in pull request #7405:
URL: https://github.com/apache/trafficserver/pull/7405#discussion_r560722534
##########
File path: iocore/net/SSLSessionCache.cc
##########
@@ -129,51 +129,50 @@ SSLSessionBucket::insertSession(const SSLSessionID &id,
SSL_SESSION *sess, SSL *
Debug("ssl.session_cache", "Inserting session '%s' to bucket %p.", buf,
this);
}
- MUTEX_TRY_LOCK(lock, mutex, this_ethread());
- if (!lock.is_locked()) {
+ Ptr<IOBufferData> buf;
+ Ptr<IOBufferData> buf_exdata;
+ size_t len_exdata = sizeof(ssl_session_cache_exdata);
+ buf = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
+ ink_release_assert(static_cast<size_t>(buf->block_size()) >= len);
+ unsigned char *loc = reinterpret_cast<unsigned char *>(buf->data());
+ i2d_SSL_SESSION(sess, &loc);
+ buf_exdata = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
Review comment:
How about sharing one big IOBufferData between `loc` and `exdata` if the
both lifetimes are the same? The difference may be very small but it would be
better.
##########
File path: iocore/net/SSLUtils.cc
##########
@@ -191,6 +191,12 @@ ssl_get_cached_session(SSL *ssl, const unsigned char *id,
int len, int *copy)
static int
ssl_new_cached_session(SSL *ssl, SSL_SESSION *sess)
{
+#if OPENSSL_VERSION_NUMBER >= 0x010101000L
Review comment:
`OPENSSL_VERSION_NUMBER` will be deprecated on OpenSSL 3.0, and
BoringSSL supports these functions despite its `OPENSSL_VERSION_NUMBER` is
`0x1010007f`.
Let's check availability of the functions.
##########
File path: iocore/net/SSLSessionCache.cc
##########
@@ -129,51 +129,50 @@ SSLSessionBucket::insertSession(const SSLSessionID &id,
SSL_SESSION *sess, SSL *
Debug("ssl.session_cache", "Inserting session '%s' to bucket %p.", buf,
this);
}
- MUTEX_TRY_LOCK(lock, mutex, this_ethread());
- if (!lock.is_locked()) {
+ Ptr<IOBufferData> buf;
+ Ptr<IOBufferData> buf_exdata;
+ size_t len_exdata = sizeof(ssl_session_cache_exdata);
+ buf = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
+ ink_release_assert(static_cast<size_t>(buf->block_size()) >= len);
+ unsigned char *loc = reinterpret_cast<unsigned char *>(buf->data());
+ i2d_SSL_SESSION(sess, &loc);
+ buf_exdata = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
+ ink_release_assert(static_cast<size_t>(buf_exdata->block_size()) >=
len_exdata);
+ ssl_session_cache_exdata *exdata = reinterpret_cast<ssl_session_cache_exdata
*>(buf_exdata->data());
+ // This could be moved to a function in charge of populating exdata
+ exdata->curve = (ssl == nullptr) ? 0 : SSLGetCurveNID(ssl);
+ ink_hrtime now = Thread::get_hrtime_updated();
+
+ ats_scoped_obj<SSLSession> ssl_session(new SSLSession(id, buf, len,
buf_exdata, now));
Review comment:
I'm surprised we use `new` for this frequently created object. Since
it's before acquiring lock there would be less impact on lock contention, but
we may want to avoid real memory allocation here.
##########
File path: iocore/net/SSLSessionCache.h
##########
@@ -134,18 +166,19 @@ class SSLSessionBucket
public:
SSLSessionBucket();
~SSLSessionBucket();
- void insertSession(const SSLSessionID &, SSL_SESSION *ctx, SSL *ssl);
- bool getSession(const SSLSessionID &, SSL_SESSION **ctx,
ssl_session_cache_exdata **data);
- int getSessionBuffer(const SSLSessionID &, char *buffer, int &len);
- void removeSession(const SSLSessionID &);
+ void insertSession(const SSLSessionID &sid, SSL_SESSION *sess, SSL *ssl);
+ bool getSession(const SSLSessionID &sid, SSL_SESSION **sess,
ssl_session_cache_exdata **data);
+ int getSessionBuffer(const SSLSessionID &sid, char *buffer, int &len);
+ void removeSession(const SSLSessionID &sid);
private:
/* these method must be used while hold the lock */
void print(const char *) const;
- void removeOldestSession();
+ void removeOldestSession(const std::unique_lock<std::shared_mutex> &lock);
- Ptr<ProxyMutex> mutex;
- CountQueue<SSLSession> queue;
+ mutable std::shared_mutex mutex;
+ std::unordered_map<SSLSessionID, SSLSession *, SSLSessionIDHash> bucket_data;
+ std::map<ink_hrtime, SSLSession *> bucket_data_ts;
Review comment:
I know `std::map` sorts items by key, but like I pointed out on my
comment above, I don't think eviction order have to be that accurate. How about
using `std::deque` instead?
##########
File path: iocore/net/SSLSessionCache.cc
##########
@@ -129,51 +129,50 @@ SSLSessionBucket::insertSession(const SSLSessionID &id,
SSL_SESSION *sess, SSL *
Debug("ssl.session_cache", "Inserting session '%s' to bucket %p.", buf,
this);
}
- MUTEX_TRY_LOCK(lock, mutex, this_ethread());
- if (!lock.is_locked()) {
+ Ptr<IOBufferData> buf;
+ Ptr<IOBufferData> buf_exdata;
+ size_t len_exdata = sizeof(ssl_session_cache_exdata);
+ buf = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
+ ink_release_assert(static_cast<size_t>(buf->block_size()) >= len);
+ unsigned char *loc = reinterpret_cast<unsigned char *>(buf->data());
+ i2d_SSL_SESSION(sess, &loc);
+ buf_exdata = new_IOBufferData(buffer_size_to_index(len,
MAX_BUFFER_SIZE_INDEX), MEMALIGNED);
+ ink_release_assert(static_cast<size_t>(buf_exdata->block_size()) >=
len_exdata);
+ ssl_session_cache_exdata *exdata = reinterpret_cast<ssl_session_cache_exdata
*>(buf_exdata->data());
+ // This could be moved to a function in charge of populating exdata
+ exdata->curve = (ssl == nullptr) ? 0 : SSLGetCurveNID(ssl);
+ ink_hrtime now = Thread::get_hrtime_updated();
Review comment:
Do we really need to care creation time that much? I don't think
eviction order have to be that accurate.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
