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

Synchronous Socket: unnecessary skb postponing #80

Closed
krizhanovsky opened this issue Mar 18, 2015 · 1 comment
Closed

Synchronous Socket: unnecessary skb postponing #80

krizhanovsky opened this issue Mar 18, 2015 · 1 comment
Assignees

Comments

@krizhanovsky
Copy link
Contributor

Based on Alexey's notes.

ss_tcp_process_proto_skb() calls postpone_skb() callback if connection_recv() returns SS_POSTPONE. postpone_skb() points to tfw_connection_postpone_skb() which just adds the skb to conn->msg->skb_list. Meantime, ss_tcp_process_skb() firstly calls put_skb_to_msg() callback before executing ss_tcp_process_proto_skb() and the callback points to tfw_connection_put_skb_to_msg() which also adds the skb to the same list. So postponing the skb has no sense.

@keshonok
Copy link
Contributor

That is correct summary of my notes. Closely related is issue #31.

An incoming SKB is processed by ss_tcp_process_skb(). First thing it does is call put_skb_to_msg() callback that points to tfw_connection_put_skb_to_msg(). That function creates a new connection message if needed, and then adds the SKB to that message's list conn->msg->skb_list by executing ss_skb_queue_tail(&conn->msg->skb_list, skb);

Then ss_tcp_process_proto_skb() is called on every piece of data in the SKB. In turn, it calls the connection_recv() callback. On TFW_POSTPONE result from connection_recv() it calls postpone_skb() callback that points tfw_connection_postpone_skb(). The only purpose of tfw_connection_postpone_skb() is executing ss_skb_queue_tail(&conn->msg->skb_list, skb); - again, on the same SKB, the same message, and the same connection.

Granted, that doesn't do any harm as the SKB is not actually added to the same list twice due to safely measures.

All of that means the following:

  • The whole if (r == SS_POSTPONE) case can safely be removed from ss_tcp_process_proto_skb(). That will eliminate unnecesary multiple calls to tfw_connection_postpone_skb().
  • .postpone hook in ssocket_hooks{} is not needed as well.
  • The whole tfw_connection_postpone_skb() function can safely be removed as unused and unnecessary.

@krizhanovsky krizhanovsky modified the milestones: 0.5.0 SSL, Stable, 0.3.0 Proxy Filter May 4, 2015
@krizhanovsky krizhanovsky modified the milestones: 0.4.0 Web Server, 0.3.0 Proxy Filter May 24, 2015
@krizhanovsky krizhanovsky removed their assignment May 31, 2015
@keshonok keshonok self-assigned this Jun 26, 2015
keshonok added a commit that referenced this issue Jun 26, 2015
Due to specifics of SKB processing in Tempesta postponing has become
a dummy operation that does nothing and is completely unnecessary.
keshonok added a commit that referenced this issue Jun 27, 2015
TFW_POSTPONE status of HTTP data parsing means that parsing succeeded but
additional data is needed to complete it. This status only makes sense in
upper layers that process the results of parsing and act on it. Lower layers
just supply data. They only want to know if processing of a message should
be continued or not.
keshonok added a commit that referenced this issue Jun 27, 2015
TFW_POSTPONE status of HTTP data parsing means that parsing succeeded but
additional data is needed to complete it. This status only makes sense in
upper layers that process the results of parsing and act on it. Lower layers
just supply data. They only want to know if processing of a message should
be continued or not.
keshonok added a commit that referenced this issue Jun 29, 2015
TFW_POSTPONE status of HTTP data parsing means that parsing succeeded
but more data is needed to complete it. This status only makes sense
in upper layers that process the results of parsing and act upon it.
Lower layers just supply data for parsing. They only want to know
if processing of a message should continue or not.
keshonok added a commit that referenced this issue Jun 29, 2015
TFW_POSTPONE status of HTTP data parsing means that parsing succeeded
but more data is needed to complete it. This status only makes sense
in upper layers that process the results of parsing and act upon it.
Lower layers just supply data for parsing. They only want to know
if processing of a message should continue or not.
krizhanovsky added a commit that referenced this issue Jun 29, 2015
Eliminate unnecessary SKB postponing (#80)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants