Changeset View
Standalone View
sys/kern/uipc_ktls.c
Show First 20 Lines • Show All 293 Lines • ▼ Show 20 Lines | ktls_crypto_backend_deregister(struct ktls_crypto_backend *be) | ||||
} | } | ||||
LIST_REMOVE(be, next); | LIST_REMOVE(be, next); | ||||
rm_wunlock(&ktls_backends_lock); | rm_wunlock(&ktls_backends_lock); | ||||
return (0); | return (0); | ||||
} | } | ||||
#if defined(INET) || defined(INET6) | #if defined(INET) || defined(INET6) | ||||
static uint16_t | static u_int | ||||
ktls_get_cpu(struct socket *so) | ktls_get_cpu(struct socket *so) | ||||
{ | { | ||||
struct inpcb *inp; | struct inpcb *inp; | ||||
uint16_t cpuid; | u_int cpuid; | ||||
inp = sotoinpcb(so); | inp = sotoinpcb(so); | ||||
#ifdef RSS | #ifdef RSS | ||||
cpuid = rss_hash2cpuid(inp->inp_flowid, inp->inp_flowtype); | cpuid = rss_hash2cpuid(inp->inp_flowid, inp->inp_flowtype); | ||||
if (cpuid != NETISR_CPUID_NONE) | if (cpuid != NETISR_CPUID_NONE) | ||||
return (cpuid); | return (cpuid); | ||||
jhb: The result is stored in a uint16_t value, so I'm not sure this change is a good idea as it will… | |||||
bzAuthorUnsubmitted Done Inline ActionsThis is not true; the result is stored in tls->wq_index = ktls_get_cpu(so); which I is defined here as u_int: sys/sys/ktls.h: u_int wq_index; The hpts code is a mess and probably not consistent in a couple of places. I am not going near it beyond making it compile. Maybe the NETISR defines should not be re-used or we should re-define them then? I think the original "int" came from sys/sys/smp.h (mp_maxid et al) and related and sys/sys/pcpu.h: u_int pc_cpuid; /* This cpu number */. bz: This is not true; the result is stored in
tls->wq_index = ktls_get_cpu(so);
which I is… | |||||
jhbUnsubmitted Not Done Inline ActionsHuh, so it is. I think this is ok. ktls_cpuid_lookup[] is probably packed this way for the cache reason (it was that way when I first got the code from Netflix), though it's not clear to me that it matters, but I think your change as is is fine for KTLS. I can't speak to the hpts bits. jhb: Huh, so it is. I think this is ok. ktls_cpuid_lookup[] is probably packed this way for the… | |||||
gallatinUnsubmitted Not Done Inline ActionsIt was just cut-and-pasted from hpts gallatin: It was just cut-and-pasted from hpts | |||||
jhbUnsubmitted Not Done Inline ActionsAh, I'd actually prefer to make it an array of u_ints if that is ok with you then. jhb: Ah, I'd actually prefer to make it an array of `u_int`s if that is ok with you then. | |||||
rrsUnsubmitted Not Done Inline ActionsThe reason it was a uint16_t is that I was trying to fit as much as possible in a cache line, as you state. And like you rrs: The reason it was a uint16_t is that I was trying to fit as much as possible in a cache line… | |||||
bzAuthorUnsubmitted Done Inline Actions@rrs the real problem is in HPTS. There I cannot as easily make it an int everywhere where it should be currently. Can you have a look at that code and see if it is ok in that case there? bz: @rrs the real problem is in HPTS. There I cannot as easily make it an int everywhere where it… | |||||
#endif | #endif | ||||
/* | /* | ||||
* Just use the flowid to shard connections in a repeatable | * Just use the flowid to shard connections in a repeatable | ||||
* fashion. Note that some crypto backends rely on the | * fashion. Note that some crypto backends rely on the | ||||
* serialization provided by having the same connection use | * serialization provided by having the same connection use | ||||
* the same queue. | * the same queue. | ||||
*/ | */ | ||||
cpuid = ktls_cpuid_lookup[inp->inp_flowid % ktls_number_threads]; | cpuid = ktls_cpuid_lookup[inp->inp_flowid % ktls_number_threads]; | ||||
▲ Show 20 Lines • Show All 1,265 Lines • Show Last 20 Lines |
The result is stored in a uint16_t value, so I'm not sure this change is a good idea as it will just silently truncate. I think instead you want to do something where you KASSERT that the value is actually not larger than UINT16_MAX and have the function still return uint16_t. You probably want to do something similar in the hpts code. The reason for the uint16_t is to fit more bits into a single cache line, and FreeBSD doesn't run on any systems where the number of CPUs even remotely approach 2^16.