Page MenuHomeFreeBSD

[ng_socket] Don't take the SOCKBUF_LOCK() twice in the RX data path.
ClosedPublic

Authored by afedorov on Nov 26 2020, 1:24 PM.

Details

Summary

This is just a minor optimization, but it's sensitive. This gives an improvement of 30-50 kpps.

Description:
sbappendaddr() takes the lock internally.
sorwakeup() also takes the lock internally.
So, the lock is taken twice.

ngs_rcvmsg () already does this in the manner suggested by the patch.

Test Plan

We have been using these changes for over six months without any visible issues.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

I'm not particularly familiar with this code, but the patch does look correct to me.

sys/netgraph/ng_socket.c
1036 ↗(On Diff #80016)

That might want a comment to explain that sorwakeup_locked() releases the lock.

Add a comment about locking.

afedorov added inline comments.
sys/netgraph/ng_socket.c
1036 ↗(On Diff #80016)

Yes, I thought about it)) While I'm here, add the same comment to ngs_rcvmsg().

markj added inline comments.
sys/netgraph/ng_socket.c
1032 ↗(On Diff #80019)

Indentation on the second line should be by four spaces.

This revision is now accepted and ready to land.Nov 26 2020, 2:51 PM
afedorov marked an inline comment as done.

Fix indentation.

This revision now requires review to proceed.Nov 26 2020, 2:58 PM
This revision is now accepted and ready to land.Nov 26 2020, 3:21 PM

This patch seems to complicate things at the first glance.
But in the end, the sowakeup() call itself is the problem.
It requires to be called while holding the lock and releases it.

Usually lock should be hold as short as possible, so the function ngs_recmsg() seems to hold the lock unnecessary long. But there might be other races.

So, okay from me.

Not familiar with the code, but it looks ok to me.