Page MenuHomeFreeBSD

socket: introduction of destructor callback
ClosedPublic

Authored by micchie.gml_gmail.com on Jun 8 2018, 3:00 PM.

Details

Summary

A netmap extension to use the kernel stack (introduced in BSDCan 2018),
associates a socket with netmap port.
Thus, netmap would like to be notified of the socket close().

Test Plan

Diff Detail

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

Event Timeline

jtl added a subscriber: jtl.Jun 8 2018, 3:26 PM
jtl added inline comments.
sys/kern/uipc_socket.c
1104 ↗(On Diff #43458)

Is this the correct place to call the destructor? In this location, the destructor would get called when the user closes the socket; however, the protocol level can still hold a reference, which will delay the actual "destruction" of the socket (which, arguably, occurs in sofree).

sys/sys/socketvar.h
114 ↗(On Diff #43458)

Is this required for both regular and listening sockets? If not, it should go in the one sub-structure that needs it.

jtl added a reviewer: network.Jun 8 2018, 3:26 PM
jtl accepted this revision.Jun 8 2018, 6:38 PM

The submitter spoke to me in person at BSDCan and answered my questions.

sys/kern/uipc_socket.c
1104 ↗(On Diff #43458)

The submitter says the purpose is to be notified of the user's close() call, so this does indeed seem like the correct place to make the call.

sys/sys/socketvar.h
114 ↗(On Diff #43458)

The submitter says this is applicable to both, so should be in the general section. I'll probably move it to be adjacent to so_emuldata to limit the possibility we create new alignment padding.

This revision is now accepted and ready to land.Jun 8 2018, 6:38 PM
This revision was automatically updated to reflect the committed changes.

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

jtl added a comment.Jun 15 2018, 6:19 PM

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

Yes, I caught that when I asked the submitter about the placement of the "destructor". But, once I figured out it was a close notification, I should have changed the name. Mea cupla.

Before changing this, let me see if I can confirm what the Linux implementation does.

In D15706#334711, @jtl wrote:

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

Yes, I caught that when I asked the submitter about the placement of the "destructor". But, once I figured out it was a close notification, I should have changed the name. Mea cupla.
Before changing this, let me see if I can confirm what the Linux implementation does.

Thanks, it is intended to be equivalent to sk_destruct() callback in struct sock in Linux.

In D15706#334711, @jtl wrote:

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

Yes, I caught that when I asked the submitter about the placement of the "destructor". But, once I figured out it was a close notification, I should have changed the name. Mea cupla.
Before changing this, let me see if I can confirm what the Linux implementation does.

Thanks, it is intended to be equivalent to sk_destruct() callback in struct sock in Linux.

Is the Linux sk_destruct called on socket close, or on socket destruction? If we want an actual socket destructor, we can add one of those [instead / as well], called from the socket destructor function.

In D15706#334711, @jtl wrote:

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

Yes, I caught that when I asked the submitter about the placement of the "destructor". But, once I figured out it was a close notification, I should have changed the name. Mea cupla.
Before changing this, let me see if I can confirm what the Linux implementation does.

Thanks, it is intended to be equivalent to sk_destruct() callback in struct sock in Linux.

Is the Linux sk_destruct called on socket close, or on socket destruction? If we want an actual socket destructor, we can add one of those [instead / as well], called from the socket destructor function.

It is literally destructor, which is called when the final reference count drops to zero. So probably we should define so_dtor() as destructor (so keep the name) and call it after SOCK_UNLOCK() in sofree(). (in Linux it is invoked without locking socket).

jtl added a comment.Jun 18 2018, 11:07 PM
In D15706#334711, @jtl wrote:

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

Yes, I caught that when I asked the submitter about the placement of the "destructor". But, once I figured out it was a close notification, I should have changed the name. Mea cupla.
Before changing this, let me see if I can confirm what the Linux implementation does.

Thanks, it is intended to be equivalent to sk_destruct() callback in struct sock in Linux.

Is the Linux sk_destruct called on socket close, or on socket destruction? If we want an actual socket destructor, we can add one of those [instead / as well], called from the socket destructor function.

It is literally destructor, which is called when the final reference count drops to zero. So probably we should define so_dtor() as destructor (so keep the name) and call it after SOCK_UNLOCK() in sofree(). (in Linux it is invoked without locking socket).

Which behavior do you want? As @rwatson indicated, you can both have a destructor and a close callback.

In D15706#336214, @jtl wrote:
In D15706#334711, @jtl wrote:

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

Yes, I caught that when I asked the submitter about the placement of the "destructor". But, once I figured out it was a close notification, I should have changed the name. Mea cupla.
Before changing this, let me see if I can confirm what the Linux implementation does.

Thanks, it is intended to be equivalent to sk_destruct() callback in struct sock in Linux.

Is the Linux sk_destruct called on socket close, or on socket destruction? If we want an actual socket destructor, we can add one of those [instead / as well], called from the socket destructor function.

It is literally destructor, which is called when the final reference count drops to zero. So probably we should define so_dtor() as destructor (so keep the name) and call it after SOCK_UNLOCK() in sofree(). (in Linux it is invoked without locking socket).

Which behavior do you want? As @rwatson indicated, you can both have a destructor and a close callback.

Just the destructor behaviour should suffice, but let me come back soon after checking.

In D15706#336214, @jtl wrote:
In D15706#334711, @jtl wrote:

Didn't quite catch this before it was committed. This isn't really a destructor, it's a close notification. Rather than confuse matters, as sockets have UMA destructors as well, this should probably be so_notify_close.

Yes, I caught that when I asked the submitter about the placement of the "destructor". But, once I figured out it was a close notification, I should have changed the name. Mea cupla.
Before changing this, let me see if I can confirm what the Linux implementation does.

Thanks, it is intended to be equivalent to sk_destruct() callback in struct sock in Linux.

Is the Linux sk_destruct called on socket close, or on socket destruction? If we want an actual socket destructor, we can add one of those [instead / as well], called from the socket destructor function.

It is literally destructor, which is called when the final reference count drops to zero. So probably we should define so_dtor() as destructor (so keep the name) and call it after SOCK_UNLOCK() in sofree(). (in Linux it is invoked without locking socket).

Which behavior do you want? As @rwatson indicated, you can both have a destructor and a close callback.

Just the destructor behaviour should suffice, but let me come back soon after checking.

I confirmed that the destructor behavior i.e., invoke it after SOCK_UNLOCK() in sofree() as below.
Or should I submit a new patch?

--- a/sys/kern/uipc_socket.c
+++ b/sys/kern/uipc_socket.c
@@ -1025,6 +1025,9 @@ sofree(struct socket *so)
                so->so_error = ECONNABORTED;
        SOCK_UNLOCK(so);
 
+       if (so->so_dtor != NULL)
+               so->so_dtor(so);
+
        VNET_SO_ASSERT(so);
        if (pr->pr_flags & PR_RIGHTS && pr->pr_domain->dom_dispose != NULL)
                (*pr->pr_domain->dom_dispose)(so);
@@ -1101,8 +1104,6 @@ soclose(struct socket *so)
 drop:
        if (so->so_proto->pr_usrreqs->pru_close != NULL)
                (*so->so_proto->pr_usrreqs->pru_close)(so);
-       if (so->so_dtor != NULL)
-               so->so_dtor(so);
 
        SOCK_LOCK(so);
        if ((listening = (so->so_options & SO_ACCEPTCONN))) {