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

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

Details

Reviewers

vmaffione
jhb
glebius
markj
donner
kp

Group Reviewers

network

Commits

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

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

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

afedorov created this revision.Nov 26 2020, 1:24 PM

Herald added a subscriber: melifaro. · View Herald TranscriptNov 26 2020, 1:24 PM

afedorov requested review of this revision.Nov 26 2020, 1:24 PM

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

sys/netgraph/ng_socket.c
1038–1040	That might want a comment to explain that sorwakeup_locked() releases the lock.

Add a comment about locking.

afedorov marked an inline comment as done.Nov 26 2020, 2:36 PM

afedorov added inline comments.

sys/netgraph/ng_socket.c
1038–1040	Yes, I thought about it)) While I'm here, add the same comment to ngs_rcvmsg().

markj accepted this revision.Nov 26 2020, 2:51 PM

markj added inline comments.

sys/netgraph/ng_socket.c
1032	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

Fix indentation.

This revision now requires review to proceed.Nov 26 2020, 2:58 PM

afedorov marked an inline comment as done.Nov 26 2020, 2:58 PM

kp accepted this revision as: kp.Nov 26 2020, 3:21 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.

glebius accepted this revision.Dec 2 2020, 4:45 PM

Closed by commit rS368727: [ng_socket] Don't take the SOCKBUF_LOCK() twice in the RX data path. (authored by afedorov). · Explain WhyDec 17 2020, 6:15 PM

This revision was automatically updated to reflect the committed changes.

afedorov added a commit: rS368727: [ng_socket] Don't take the SOCKBUF_LOCK() twice in the RX data path..

Herald added a subscriber: imp. · View Herald TranscriptDec 17 2020, 6:15 PM