Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

switch from sk_destruct to TfwConnection->destruct #59

Closed
wants to merge 1 commit into from

Conversation

vdmit11
Copy link
Contributor

@vdmit11 vdmit11 commented Feb 20, 2015

Before the commit TfwConnection-related code was overwriting the
sk_destruct field of struct sock with a custom destructor function.
The original sk_destruct function was saved in TfwConnection and
had to be called manually from the custom destructor.

This change simply moves the destructor from sk_destruct to the
TfwConnection->destruct field. Now you don't need to call the
original socket destructor manually, and the field was occupied
anyway, so this commit makes code simpler for free.

Before the commit TfwConnection-related code was overwriting the
sk_destruct field of struct sock with a custom destructor function.
The original sk_destruct function was saved in TfwConnection and
had to be called manually from the custom destructor.

This change simply moves the destructor from sk_destruct to the
TfwConnection->destruct field. Now you don't need to call the
original socket destructor manually, and the field was occupied
anyway, so this commit makes code simpler for free.
@keshonok
Copy link
Contributor

My opinion is that this patch is premature and doesn't solve the core problem - that sockets are not destroyed properly, and an sk_destruct function is not called as it should.

The technique where the original destructor function is replaced with a new one, and the new function needs to call the original when it's run - that is a pretty standard technique called chaining. This technique is used throughout the linux kernel as well.

The original problem still stands. We need to solve the original problem.

@keshonok
Copy link
Contributor

After further research I believe that indeed it's better to do it this way. We don't even need to call it a destructor. The major point here is that we want to free Tempesta resources as soon as a socket leaves the ESTABLISHED state and starts the closing process. We don't really care how long the final TCP handshakes are gonna be for, or how long it would take the kernel to finally close a socket.

Socket's sk_destruct is a different matter that needs to be solved separately, and it doesn't affect the release of socket-related resources in Tempesta.

@krizhanovsky
Copy link
Contributor

The patch beacks socket-connection linkage. Previously if somebody calls sk_free(), then the connection destructor is called. Now this is not the case.

Linked with #23 somewhat.

@vdmit11
Copy link
Contributor Author

vdmit11 commented Mar 10, 2015

Previously if somebody calls sk_free(), then the connection destructor is called. Now this is not the case.

Now the destructor is called from ss_do_close() which is called when the socket leaves the TCP_ESTABLISHED state, so the Tempesta FW frees its resources early and doesn't wait until the socket is actually deallocated. Isn't it the right way to go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants