Page MenuHomeFreeBSD

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

Authored by glebius on Aug 12 2022, 8:17 AM.
Tags
None
Referenced Files
F102669150: D36164.diff
Fri, Nov 15, 3:07 PM
Unknown Object (File)
Wed, Oct 30, 2:15 PM
Unknown Object (File)
Wed, Oct 30, 5:37 AM
Unknown Object (File)
Mon, Oct 21, 6:57 AM
Unknown Object (File)
Mon, Oct 21, 6:57 AM
Unknown Object (File)
Mon, Oct 21, 6:57 AM
Unknown Object (File)
Fri, Oct 18, 12:41 PM
Unknown Object (File)
Fri, Oct 18, 12:39 PM

Details

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 46888
Build 43777: arc lint + arc unit

Event Timeline

tuexen added a subscriber: tuexen.

OK for SCTP.

sys/netinet/tcp_subr.c
1451

You might move up the definition of tcp_drain() and don't need an extra declaration anymore.

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?

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!

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

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.

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

This revision is now accepted and ready to land.Aug 13 2022, 7:48 PM
This revision now requires review to proceed.Aug 16 2022, 10:13 PM
This revision is now accepted and ready to land.Aug 17 2022, 6:15 PM