Page MenuHomeFreeBSD

Make RSS kernels compile again
ClosedPublic

Authored by bz on Feb 17 2020, 11:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 13, 1:11 PM
Unknown Object (File)
Sat, Apr 13, 7:53 AM
Unknown Object (File)
Thu, Apr 11, 7:03 PM
Unknown Object (File)
Jan 17 2024, 6:08 PM
Unknown Object (File)
Jan 13 2024, 11:20 AM
Unknown Object (File)
Jan 2 2024, 5:14 PM
Unknown Object (File)
Jan 2 2024, 10:17 AM
Unknown Object (File)
Dec 23 2023, 1:18 AM

Details

Summary

(Partially) fix type sizes and includes for RSS kernels
to compile agian. For as long as we use netisr CPU defines
we cannot use uin16_t. tcp_hpts.c and all related header
files however do that everywhere. This needs to be cleaned
up and fixed still.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 29444
Build 27321: arc lint + arc unit

Event Timeline

sys/netinet/tcp_hpts.c
1176

I am aware that there is some whitspace noise in here as well...

donner added inline comments.
sys/netinet/tcp_hpts.c
1158

should this also be u_int?

bz marked an inline comment as done.Feb 17 2020, 4:05 PM
bz added inline comments.
sys/netinet/tcp_hpts.c
1158

Yes in theory and in a lot more places that will be on the trail of all this. That's why I mentioned that it needs to be cleaned up.
The purpose of this change simply is to get an RSS kernel to compile again and not fix every bit of every part of the sources. The authors of that code can do that better than me.

bz marked an inline comment as done.Feb 24 2020, 7:17 PM

Anyone any comment on this? Otherwise I'll commit it the next two days.

sys/kern/uipc_ktls.c
312

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.

sys/kern/uipc_ktls.c
312

This 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 */.

sys/kern/uipc_ktls.c
312

Huh, 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.

Can someone look at the hpts bits please? I know they are not as straight forward as is the ktls case.

gallatin added inline comments.
sys/kern/uipc_ktls.c
312

It was just cut-and-pasted from hpts

This revision is now accepted and ready to land.Feb 27 2020, 3:20 PM
sys/kern/uipc_ktls.c
312

Ah, I'd actually prefer to make it an array of u_ints if that is ok with you then.

sys/kern/uipc_ktls.c
312

The 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
I felt that 2^16 cpu's is a few years away. Can we make it an int, sure..

sys/kern/uipc_ktls.c
312

@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?

This revision was automatically updated to reflect the committed changes.

I am re-opening this again as HPTS still needs fixing @rrs . See https://reviews.freebsd.org/D23726?id=68452 for the original diff.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 3 2020, 2:15 PM
This revision was automatically updated to reflect the committed changes.