User Details
- User Since
- May 28 2014, 2:27 PM (640 w, 6 d)
Today
Since proposed changes all happen under RSS, make Adrian the owner, but also subscribe other potentially interested parties.
Pardon naive quick question, before reading deeper into the proposal: any downsides with proposed behavior? Can it be on by default for SO_REUSEPORT_LB?
Any opinion if NOLOCK would be a better name than NULLLOCK?
Yesterday
Rebase on new version of D59457.
- Keep br_lock in user mode.
- parenthesis
Thanks to Nick for pointing out the problem, that I had a blind eye on.
I have no objections on the change, although I would do it differently.
- Use lk2lo() and NULLLOCK.
- Manual page fixes.
- Use lk2lo().
Sun, Sep 6
Remove mention of lock_object_t in the manual page.
- Use _Generic() instead of transparent union.
- Reduce amount of changes.
D59456 refactored to use _Generic()
- Use _Generic() instead of transparent union
- Cover dummynet, too.
I'm in favor of using MSG_WAITALL as it makes applications to do less work. Given that hastd protocol can predict when we are expecting to receive a descriptor, the patch like in D57511 seems a good solution for me. The problem I see in D57511 is that in the proto_descriptor_recv() the size of data is not checked, just asked to be > 0. It probably can be combined with your change that changes return values to ssize_t.
I can't see how _Generic() can do the same. AFAIK, _Generic() implies multiple different functions. My goal is the opposite - have single function that accepts arguments of different (but compatible) types.
Sat, Sep 5
- Use IF_MAXMTU.
- Provide cache linkage, so that obsolete cache holds a pointer to a new one.
- Add sysctl to disable the feature.
- No local variable.
- No local variable at all.
I vaguely remember that December 2025 when I was refactoring the attach/detach of domains/interfaces 0d469d23715d690b863787ebfa51529e1f6a9092, I found out that there are at least 2 places where we free addresses and multicast addresses. And apparently we mishandle that as there is a leak left. And I left it for later. Maybe good time for another take on that mess.
Fri, Sep 4
Treat this as a suggestion. IMHO, this is more readable than the old version. If everybody disagrees with me, I'm fine with abandoning this.
Alternative https://reviews.freebsd.org/D59426 I will also see if the new PULLUP() can be used everywhere down below the function to easily get typed pointers with guaranteed memory length provided.
Ok, this mess mostly comes from me. While reviewing this patch I came to alternative that I will post soon.
P.S. Can't resist to put my standard sidenote here. This all is so complicated due to if_vmove() :)
You definitely found the problem, but the fix doesn't seem correct. There is asymmetry in allocation/freeing. It could close the leak at some configurations without regressions, of course. But IMHO not at all configurations.
Thu, Sep 3
I added Dag-Erling as he recently committed to hastd.
Wed, Sep 2
I'm also hitting deadlock on boot on one of my desktops. And this patch helps. And of course it makes things correct.
Tue, Sep 1
I'd be happy if you go back to original version of this revision. Sorry for incorrectly reading the file and not understanding that both uses of IFNET_WLOCK were actually connected and both originating from the netmap change.
Did the original version of this review removed use of IFNET_WLOCK completely? If I understand phabricator correct, that would be URL https://reviews.freebsd.org/D59294?id=185524 I also would prefer this.
Mon, Aug 31
Please refactor the fe00ca2f88718 and I will see if anything can be done to e0e12405285b6. If successful with the latter, I will move the lock to if.c and that will prevent LLMs to produce this kind of problem again.
Mkay. We were actually already 100% correct on this lock until in 2022 in e0e12405285b6 vmaffione@ abused it for the first time. That wasn't noticed, since that was specific to netmap and due to me not keeping eye on changes close back in 2022. And very recently you pushed some LLM generated code in fe00ca2f88718 that introduced more IFNET_WLOCK() into iflib.c. Most likely you are stepping on the latter.
It is a problem of iflib. Drivers shall not use IFNET_WLOCK(). Ideally the lock should be private to net/if.c, in reality some other files in the network stack use it, but not drivers. iflib violates that, and it seems to be a pretty recent thing.
I've been running with this patch for 4 days. Not watchdog timeouts observed.
Thu, Aug 27
Tue, Aug 25
All binaries. As I tried to explain on the lists a while back, a python script like:
with socket.socket(socket.AF_INET, socket.SOCK_RAW, IPPROTO_DIVERT) as fd: fd.bind(('0.0.0.0', 8000)) r = fd.recvfrom(8192) ... fd.sendto(r[0], r[1])does not work properly without this change. Changing the socket domain to PF_DIVERT does not fix the problem, because python does not know which sockaddr to use for recvfrom in that case.
