Hi!
On 12/12/2013 08:39 PM, [email protected] wrote:
> Hi!
>> This is a perfomance test for TCP Fast Open (TFO) which is an extension to
>> speed up the opening of TCP connections between two endpoints. It reduces
>> the number of round time trips (RTT) required in TCP conversations. TFO could
>> result in speed improvements of between 4% and 41% in the page load times on
>> popular web sites.
>>
>> The default test scenario simulates an average conversation between a
>> web-browser and an application server, so the test results with TFO enabled
>> must be at least 3 percent faster.
>>
>> The test must be run on Linux versions higher then 3.7. (TFO client side
>> implemented in Linux 3.6, server side in Linux 3.7).
>>
>> Signed-off-by: Alexey Kodanev<[email protected]>
>> ---
>> testcases/network/tcp_fastopen/.gitignore | 1 +
>> testcases/network/tcp_fastopen/Makefile | 24 +
>> testcases/network/tcp_fastopen/README | 16 +
>> testcases/network/tcp_fastopen/tcp_fastopen.c | 783
>> ++++++++++++++++++++
>> testcases/network/tcp_fastopen/tcp_fastopen_run.sh | 164 ++++
>> 5 files changed, 988 insertions(+), 0 deletions(-)
>> create mode 100644 testcases/network/tcp_fastopen/.gitignore
>> create mode 100644 testcases/network/tcp_fastopen/Makefile
>> create mode 100644 testcases/network/tcp_fastopen/README
>> create mode 100644 testcases/network/tcp_fastopen/tcp_fastopen.c
>> create mode 100755 testcases/network/tcp_fastopen/tcp_fastopen_run.sh
>>
>>
>> ...
>>
>> +struct tcp_func {
>> + void (*init)(void);
>> + void (*run)(void);
>> + void (*cleanup)(void);
>> +};
>> +static struct tcp_func tcp;
>> +
>> +#define MAX_THREADS 10000
>> +static pthread_attr_t attr;
>> +static pthread_t *thread_ids;
>> +static int threads_num;
>> +
>> +static struct addrinfo *remote_addrinfo;
>> +static struct addrinfo *local_addrinfo;
>> +static const struct linger clo = { 1, 3 };
>> +
>> +static void cleanup(void)
>> +{
>> + static int first = 1;
>> + if (!first)
>> + return;
>> + first = 0;
> There is still chance for a race condition here. But this could be
> easily fixed by pthread_once().
>
> static pthread_once_t cleanup_executed = PTHREAD_ONCE_INIT;
>
> static void do_cleanup(void)
> {
> /* Do the cleanup here */
> }
>
> static void cleanup(void)
> {
> pthread_once(&cleanup_executed, do_cleanup);
> }
>
> Thinking of the problems with LTP library thread safety again. One of
> the solutions may be having two tst_res.c implementaions. Note that the
> source of thread safety problems is the cleanup function which is
> executed by the tst_brkm() and the exitval that is in the test library
> too.
>
> If we did so then we will have tst_res.c we have now and one providing
> thread safe implemenation using pthread synchronization primitives
> (likely both will be implemented on the top of the common code).
>
> And if we could switch which one gets linked to a test at the linker
> phase depending whether it uses pthreads or not the thread safety
> problem will be fixed once for all.
>
> I will think about this a bit more.
As I understand correctly we need to create one more library libltp_mt.a
and implement the same but in the thread-safe way, right?
So the following part of tst_brk() also requires synchronization,
tst_brk_entered in particular.
void tst_brk(int ttype, char *fname, void (*func) (void), char *arg_fmt,
...)
{
...
/*
* If no cleanup function was specified, just return to the caller.
* Otherwise call the specified function.
*/
if (func != NULL) {
tst_brk_entered++;
(*func) ();
tst_brk_entered--;
}
if (tst_brk_entered == 0)
tst_exit();
}
tst_brk_entered and T_exitval, tst_count can be implemented with gcc
atomic bultins. Cleanup function as you suggested will be implemented
with pthread_once in a test. Then we can exit with TCONF if a test with
libltp thread-safety requirements can't find gcc atomic built-ins.
Anyway, I'm thinking that at least for now I won't use tst_res.c in the
test, once thread-safe ltplib is done I will make use of it here.
>
>> ...
>>
>> +static int client_connect_send(const char *msg, int size)
>> +{
>> + int cfd = socket(AF_INET, SOCK_STREAM, 0);
>> + const int flag = 1;
>> + setsockopt(cfd, SOL_SOCKET, SO_REUSEADDR, &flag, sizeof(flag));
>> +
>> + if (cfd == -1) {
>> + tst_resm(TWARN | TERRNO, "socket failed at %s:%d",
>> + __FILE__, __LINE__);
>> + return cfd;
>> + }
>> +
>> + if (fastopen_api == TFO_ENABLED) {
>> + /* Replaces connect() + send()/write() */
>> + if (sendto(cfd, msg, size, MSG_FASTOPEN | MSG_NOSIGNAL,
>> + remote_addrinfo->ai_addr,
>> + remote_addrinfo->ai_addrlen) != size) {
>> + tst_resm(TFAIL | TERRNO, "sendto failed");
>> + SAFE_CLOSE(NULL, cfd);
>> + return -1;
>> + }
>> + } else {
>> + /* old TCP API */
>> + if (connect(cfd, remote_addrinfo->ai_addr,
>> + remote_addrinfo->ai_addrlen)) {
>> + tst_resm(TFAIL | TERRNO, "connect failed");
>> + SAFE_CLOSE(NULL, cfd);
>> + return -1;
>> + }
>> +
>> + if (send(cfd, msg, size, MSG_NOSIGNAL) != client_msg_size) {
>> + tst_resm(TFAIL | TERRNO,
>> + "send failed on sock '%d'", cfd);
>> + SAFE_CLOSE(NULL, cfd);
>> + return -1;
>> + }
>> + }
>> +
>> + return cfd;
>> +}
>> +
>> +void *client_fn(void *arg)
>> +{
>> + char buf[server_msg_size];
>> + int cfd, i;
>> +
>> + /* connect & send requests */
>> + cfd = client_connect_send(client_msg, client_msg_size);
>> + if (cfd == -1)
>> + return NULL;
> So if the connection breaks after the server was started and the client
> fails to connect the test will run fine and the resoult would be two
> nearly identical runtimes?
In that case client will exit with error message and TFAIL exit status
(connect() or send() failed). Then tcp_fastopen_run.sh will check
client's exit value and will TBROK if the value isn't zero.
The bad thing here: it will try to close server and write result file to
no purpose.
>> ...
>>
>> +
>> +static void make_client_request(void)
>> +{
>> + client_msg[0] = start_byte;
>> +
>> + /* set size for reply */
>> + *(uint16_t *)(client_msg + 1) = htons(server_msg_size);
> This will generate unaligned access on some architectures.
>
> What you need is to set the value byte by byte:
>
> client_msg[1] = (sever_msg_size>>8) & 0xff;
> client_msg[2] = sever_msg_size & 0xff;
>
>> + client_msg[client_msg_size - 1] = end_byte;
>> +}
>> +
>> +static int parse_client_request(const char *recv_msg)
>> +{
>> + int size = ntohs(*(uint16_t *)(recv_msg + 1));
> Here as well, you need to construct the value byte by byte as:
>
> uint16_t val = htons(((((uint16_t)recv_msg[1])<<8) + recv_msg[2]));
OK, but I think it is better to use an union as follows, isn't it?
union net_size_field {
char bytes[2];
uint16_t value;
};
static void make_client_request(void)
{
...
union net_size_field net_size;
net_size.value = htons(server_msg_size);
client_msg[1] = net_size.bytes[0];
client_msg[2] = net_size.bytes[1];
..
}
static int parse_client_request(const char *recv_msg)
{
union net_size_field net_size;
net_size.bytes[0] = recv_msg[1];
net_size.bytes[1] = recv_msg[2];
int size = ntohs(net_size.value);
...
}
>> ...
>>
>> + while (1) {
>> + recv_len = sock_recv_poll(client_fd, recv_msg,
>> + max_msg_len, &offset);
>> +
>> + if (recv_len == 0) {
>> + break;
>> + } else
>> + if (recv_len < 0 || (offset + recv_len) > max_msg_len ||
>> + (recv_msg[0] != start_byte &&
>> + recv_msg[0] != start_fin_byte)) {
>> + tst_resm(TFAIL, "recv failed, sock '%d'", client_fd);
>> + break;
>> + }
> This part of the code looks wrongly indented. Is the else branch needed
> at all?
It isn't needed, I'll fix it.
>> ...
>>
>> + if (recv_msg[offset - 1] != end_byte) {
>> + tst_resm(TINFO, "msg is not complete, continue recv");
>> + continue;
>> + }
>> +
>> + if (recv_msg[0] == start_fin_byte)
>> + tst_brkm(TBROK, cleanup, "client asks to terminate...");
> I still dont like the TBROK on clean exit. Please change it to:
>
> cleanup();
> tst_exit();
>
> Actually I may be inclined to add a test exit function that would call
> cleanup first as the cleanup(); tst_exit() is quite common pattern.
>
> void tst_exit2(void (*cleanup)(void));
>
> Or something similar would do. Unfortunatelly we cannot change
> tst_exit() because changing every single usage in tests is not
> feasible.
OK, I'll replace it with the three functions:
tst_resm(TINFO,...)
cleanup()
tst_exit()
Best regards,
Alexey
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list
