Page MenuHomeFreeBSD
Differential D35300

unix/dgram: use minimal possible socket buffer for PF_UNIX/SOCK_DGRAM
ClosedPublic
Actions

Authored by glebius on May 23 2022, 9:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 5, 10:46 AM
Unknown Object (File)
Fri, Apr 5, 10:46 AM
Unknown Object (File)
Fri, Apr 5, 10:46 AM
Unknown Object (File)
Fri, Apr 5, 10:46 AM
Unknown Object (File)
Fri, Apr 5, 10:46 AM
Unknown Object (File)
Fri, Apr 5, 10:46 AM
Unknown Object (File)
Fri, Apr 5, 10:46 AM
Unknown Object (File)
Fri, Apr 5, 10:46 AM
View All 71 Files
Subscribers
imp

Details

Reviewers
markj
Group Reviewers
network
transport
Commits
rGa7444f807ec4: unix/dgram: use minimal possible socket buffer for PF_UNIX/SOCK_DGRAM
Summary

This change fully splits away PF_UNIX/SOCK_DGRAM from other socket
buffer implementations, without any behavior changes.

Generic socket implementation is reduced down to one STAILQ and very
little code.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 45828
Build 42716: arc lint + arc unit

Event Timeline

glebius created this revision.May 23 2022, 9:02 PM
Herald added a subscriber: imp. · View Herald TranscriptMay 23 2022, 9:02 PM
glebius requested review of this revision.May 23 2022, 9:02 PM
Harbormaster completed remote builds in B45681: Diff 106299.May 23 2022, 9:02 PM
glebius added a parent revision: D35299: sockets: enable protocol specific socket buffers.May 23 2022, 9:02 PM
glebius added a child revision: D35301: m_uiotombuf: write total memory length of the allocated chain in pkthdr.
glebius added a child revision: D35305: libc/syslog: deprecate use of "/var/run/logpriv".May 23 2022, 9:07 PM
glebius added reviewers: network, markj.May 23 2022, 9:12 PM
glebius added a comment.May 23 2022, 9:25 PM

Note: this patch is a step towards D35303.

markj added inline comments.May 24 2022, 3:18 PM
sys/kern/uipc_usrreq.c
1326

I'd prefer an explicit KASSERT which hints at why we expect m != NULL. Or restructure the code so that m == NULL will result in a page fault.

1417

I would make each of these cases set error and then break, then after the loop:

if (m != NULL) {
    SOCK_RECVBUF_UNLOCK(so);
    SOCK_IO_RECV_UNLOCK(so);
    return (error);
}
glebius updated this revision to Diff 106360.May 25 2022, 7:43 AM

Improve KASSERT message.

Harbormaster completed remote builds in B45709: Diff 106360.May 25 2022, 7:43 AM
glebius marked an inline comment as done.May 25 2022, 7:46 AM
glebius added inline comments.
sys/kern/uipc_usrreq.c
1417

Can you please look at the final code in D35303? With it the handling would be more comlex:

if (m == NULL && sb == NULL) {
        SOCK_RECVBUF_UNLOCK(so);
        SOCK_IO_RECV_UNLOCK(so);
        return (error)
}

If you agree with that, I can go forward and change.

markj accepted this revision as: markj.May 25 2022, 8:02 PM
markj added inline comments.
sys/kern/uipc_usrreq.c
1417

Hmm, maybe just ignore my suggestion then. We can do this kind of cleanup after the functional changes are done.

1433

I wish this code (and sbfree()) would check for wraparound with INVARIANTS on...

This revision is now accepted and ready to land.May 25 2022, 8:02 PM
glebius added a reviewer: transport.May 28 2022, 3:49 AM
glebius updated this revision to Diff 106493.May 31 2022, 3:52 AM

Rebase

This revision now requires review to proceed.May 31 2022, 3:52 AM
Harbormaster completed remote builds in B45780: Diff 106493.May 31 2022, 3:52 AM
glebius updated this revision to Diff 106588.Jun 3 2022, 2:23 AM

Add pru_sopoll method. Fixes select(2).

Harbormaster completed remote builds in B45828: Diff 106588.Jun 3 2022, 2:23 AM
markj added a comment.Jun 3 2022, 4:46 PM
In D35300#802382, @glebius wrote:

Add pru_sopoll method. Fixes select(2).

Is there an existing test case which catches this?

sys/kern/uipc_usrreq.c
1521

Does so->so_rerror need to be checked also?

1522

You also checked for SBS_CNTRCVMORE below, when POLLINIGNEOF is not set. Comparing to sopoll_generic(), the check here looks wrong.

glebius added a comment.Jun 3 2022, 7:13 PM
In D35300#802525, @markj wrote:
In D35300#802382, @glebius wrote:

Add pru_sopoll method. Fixes select(2).

Is there an existing test case which catches this?

I'm working on the event tests now. It seems I need to do substantial changes to this branch, cause there is also kevent and aio, who also just use bare sbavail().

glebius added a comment.Jun 3 2022, 7:16 PM

There are two directions to go:

  1. make protocol specific sbavail() & sbused() or soreadable() & sowriteable() and that would probably allow to leave socket poll/kevent/aio unmodified.
  2. Make protocol specific so_poll, kevent filterops and aio ops.

I'm researching these two options. Any input much appreciated.

glebius updated this revision to Diff 106635.Jun 4 2022, 7:13 PM

Abandon protocol specific select/poll for now, and instead just maintain
sb_acc + sb_ccc that generic socket code requires.

Now passes tests/kern/unix_dgram:event

Harbormaster completed remote builds in B45852: Diff 106635.Jun 4 2022, 7:13 PM
markj accepted this revision as: markj.Jun 6 2022, 1:29 PM
markj added inline comments.
sys/kern/uipc_usrreq.c
1148

BTW, you might consider asserting on valid values for flags here. For example, I forgot that PRUS_NOTREADY cannot appear here since it's only used with stream sockets.

1272
This revision is now accepted and ready to land.Jun 6 2022, 1:29 PM
glebius added inline comments.Jun 6 2022, 5:56 PM
sys/kern/uipc_usrreq.c
1148

I can do that in the changeset that adds uipc_sosend_dgram().

glebius marked an inline comment as done.Jun 6 2022, 7:36 PM
glebius added inline comments.
sys/kern/uipc_usrreq.c
1148

Nope. Those flags are not in the family of PRUS_* flags. Those are MSG_* flags. This is pru_sosend method, not pru_send. We already return error for MSG_OOB and ignore the rest. This is what original implementation does.

glebius updated this revision to Diff 106705.Jun 7 2022, 1:12 AM

Fix unp_gc_scan().

This revision now requires review to proceed.Jun 7 2022, 1:12 AM
Harbormaster completed remote builds in B45886: Diff 106705.Jun 7 2022, 1:13 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 24 2022, 4:11 PM
Closed by commit rGa7444f807ec4: unix/dgram: use minimal possible socket buffer for PF_UNIX/SOCK_DGRAM (authored by glebius). · Explain Why
This revision was automatically updated to reflect the committed changes.
glebius added a commit: rGa7444f807ec4: unix/dgram: use minimal possible socket buffer for PF_UNIX/SOCK_DGRAM.

Revision Contents
Changeset List

PathSize
sys/
kern/
212 lines
sys/
8 lines

Diff 106588

View Options

sys/kern/uipc_usrreq.c

Loading...
View Options

sys/sys/sockbuf.h

Loading...