SS: Proper TCP options #574
Conversation
| flags |= SS_SEND_F_PASS_SKB; | ||
| } else { | ||
| /* Add server connection flags */ | ||
| } |
There was a problem hiding this comment.
Perhaps, it's better just to leave a comment here and remove an empty branch.
Add server connection flags in an "else{}" branch if necessary.
| return CSTR_NEQ; | ||
| msg->flags |= TFW_HTTP_CONN_CLOSE; | ||
| tfw_http_msg_set_conn(msg, TFW_HTTP_CONN_CLOSE); |
There was a problem hiding this comment.
I don't like the patch. tfw_http_msg_conn(msg) doesn't equal to msg->flags & TFW_HTTP_CONN_KA and tfw_http_msg_set_conn(msg, TFW_HTTP_CONN_CLOSE) does more operations than msg->flags |= TFW_HTTP_CONN_CLOSE. Probably in this particular case the statements are equal and performance penalty is invisible, however the change just makes the code bit nicer (meantime msg->flags & TFW_HTTP_CONN_KA says more for me than fw_http_msg_conn(msg)) and this bit nicer doesn't outweight the badnesses.
|
"use MSG_MORE to optimize packet size at IP layer" in the original task #475 was mostly about https://github.com/tempesta-tech/linux-4.1-tfw/blob/master/net/ipv4/tcp.c#L688: don't send partial TCP segements, so it's good to implement the option. |
|
Please, review possible bug in IIRC we enter |
|
@krizhanovsky Well, |
| flags |= SS_SEND_F_CONN_CLOSE; | ||
| if (!(hm->flags & TFW_HTTP_DO_NOT_PASS)) | ||
| flags |= SS_SEND_F_PASS_SKB; | ||
| } |
There was a problem hiding this comment.
Conceptual architecture is broken. Connection layer is generic - it lays under HTTP, HTTPS and probably other protocols, so connection.[ch] shouldn't include HTTP stuff, while http.[ch] include connection.h. So firstly the change introduces circular dependency. Secondly it breaks program modules dependency.
I'd say this is HTTP responsibility to set appropriate connection flags. Moreover, tfw_connection_send() is called for server transmissions as well as for client, so there is no sense to introduce the IF construction when the caller knows exactly who it sends data to.
|
@milabs , "Well, ss_do_close always followed by the sock_put, so I do not see the problem with descriptor leakage you've mentioned. Am I missed something?" Oh, I see. Thanks. |
milabs
force-pushed
the
im-issue-475
branch
2 times, most recently
from
140c557 to
eb44494
Compare
| @@ -405,6 +402,15 @@ ss_do_close(struct sock *sk) | |||
| } | |||
| } | |||
|
|
|||
| #define __ss_sk_close(sk, lock, unlock, drop) \ | |||
There was a problem hiding this comment.
It's hard to read code like __ss_sk_close(sk, 1, 1, 1) - what the numbers are? It's hard to remember. Let's introduce defines like __ss_sk_close_locked() or __ss_sk_close_drop_locked().
This change add flags (options) to SS-operations like ss_open() and ss_close(). At the moment, only the following options have been implemented (SS_F_SYNC, SS_F_KEEP_SKB and SS_F_CONN_CLOSE), so: - SS_F_SYNC signals that synchronous operation must be performed; - SS_F_KEEP_SKB signals to ss_send(), that clones must be used and not the originals; - SS_F_CONN_CLOSE signals, that connection must be closed (droped) after the operation; Using such a flags allows us to reduce amount of arguments for ss_open and ss_close calls and also it makes it possible to remove unref_data argument from tfw_connection_send() function. Also, as the result we've use tfw_cache_add() from where it must be used and not from the http.c. Related to #475.
| if (req->flags & TFW_HTTP_CONN_CLOSE) | ||
| ((TfwMsg *)resp)->ss_flags |= SS_F_CONN_CLOSE; | ||
| } | ||
|
|
There was a problem hiding this comment.
The functions are used in http.c only, so it seems their place is in http.c.
Do we need the functions at all? E.g. __adjust_req_flags() is just single-line function which doesn't "adjust" flags, but rather set one particular flag.
There was a problem hiding this comment.
__adjust_resp_flags used twice, so it's good to have it as inline. __adjust_req_flags is used only once, but it has the same purpose as previous. I've change their names to __init_resp_ss_flags and __init_req_ss_flags.
|
good to merge |
This fixes #475