protosw: retire pr_drain and use EVENTHANDLER(9) directly
ClosedPublic
Actions

Authored by glebius on Aug 12 2022, 8:17 AM.

Details

Reviewers

tuexen
melifaro

Group Reviewers

network
transport

Commits

rG81a34d374ed6: protosw: retire pr_drain and use EVENTHANDLER(9) directly

Summary

The method was called for two different conditions: 1) the VM layer is
low on pages or 2) one of UMA zones of mbuf allocator exhausted.
This change 2) into a new event handler, but all affected network
subsystems modified to subscribe to both, so this change shall not
bring functional changes under different low memory situations.

There were three subsystems still using pr_drain: TCP, SCTP and frag6.
The latter had its protosw entry for the only reason to register its
pr_drain method.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 46890
Build 43779: arc lint + arc unit

Event Timeline

glebius created this revision.Aug 12 2022, 8:17 AM

Herald added a reviewer: transport. · View Herald TranscriptAug 12 2022, 8:17 AM

Herald added subscribers: melifaro, bz, ae, imp. · View Herald Transcript

glebius requested review of this revision.Aug 12 2022, 8:17 AM

Harbormaster completed remote builds in B46888: Diff 109230.Aug 12 2022, 8:17 AM

glebius added a parent revision: D36163: protosw: retire pr_slowtimo and pr_fasttimo.Aug 12 2022, 8:17 AM

Rebase

Harbormaster completed remote builds in B46890: Diff 109232.Aug 12 2022, 8:21 AM

OK for SCTP.

tuexen added inline comments.Aug 12 2022, 10:10 AM

sys/netinet/tcp_subr.c
1451	You might move up the definition of `tcp_drain()` and don't need an extra declaration anymore.

melifaro accepted this revision as: network, melifaro.Aug 12 2022, 11:38 AM

melifaro added inline comments.

sys/netinet/sctp_pcb.c
6972	Maybe worth entering epoch here instead? More chances of delayed epoch calls freeing memory in previously-iterated VNETs, also no need to do epoch exit in the condition above.
sys/sys/eventhandler.h
209	`mb_lowmem` and `vm_lowmem` look pretty close and and it's a bit hard to distinguish between the two (at least to me). Maybe it is worth naming this one `mbuf_lowmem` instead?

glebius added inline comments.Aug 12 2022, 3:03 PM

sys/netinet/sctp_pcb.c
6972	@tuexen Michael, since this is SCTP, what's your opinion on Alexander's suggestion?
sys/netinet/tcp_subr.c
1451	Yes, I also want to do that, but wanted to make a minimal diff. I will move it up verbatim in a separate commit.
sys/sys/eventhandler.h
209	Will do!

tuexen added inline comments.Aug 12 2022, 3:36 PM

sys/netinet/sctp_pcb.c
6972	Aren't you already entering the network epoch in line 6951 and therefore you don't need to do that here again? But I haven't really understood network epoch handling. Is there some documentation when to enter / when to leave? Right now this is mostly based on pacifying KASSERTs...

glebius added inline comments.Aug 12 2022, 5:22 PM

sys/netinet/sctp_pcb.c
6972	Alexander suggests not to enter epoch in line 6951 and instead enter multiple times, one time per vnet. The network epoch protects some of the configuration stuff in the network stack that is read very often, but doesn't change much: interface lists, address lists, etc. Subsystems are allowed to rely on the epoch for protecting there stuff. All packet processing is done in the epoch. For callouts/taskqueues/syscalls it always depends on if this particular context needs epoch or not. It actually is very possible that sctp_drain_mbufs() doesn't need epoch. But with this patch I'm preserving it since pr_drain always entered it.

tuexen added inline comments.Aug 12 2022, 8:46 PM

sys/netinet/sctp_pcb.c
6972	Alexander suggests not to enter epoch in line 6951 and instead enter multiple times, one time per vnet. The network epoch protects some of the configuration stuff in the network stack that is read very often, but doesn't change much: interface lists, address lists, etc. Subsystems are allowed to rely on the epoch for protecting there stuff. All packet processing is done in the epoch. For callouts/taskqueues/syscalls it always depends on if this particular context needs epoch or not. It actually is very possible that sctp_drain_mbufs() doesn't need epoch. But with this patch I'm preserving it since pr_drain always entered it. `sctp_drain_mbufs()` must be called from within the epoch, since it might result in sending packets. If you think that entering/leaving it is the right thing in general, do it...