Re: [LTP] [PATCH] network: tcp_fastopen: add TCP Fast Open test

[chrubis@suse.cz]

Hi!
> >> +
> >> +/* test options */
> >> +static char *narg, *Narg, *qarg, *barg, *Barg,
> >> +	*rarg, *Rarg, *aarg, *Targ;
> >> +static int nflag, Nflag, qflag, bflag, Bflag, dflag,
> >> +	rflag, Rflag, aflag, Hflag, Tflag, gflag;
> > There is no reason to use these flags. The option pointers are set to
> > NULL (global variables). So that you can use:
> >
> > 	if (arg) {
> > 		...
> > 	}
> >
> > In the check_opt() functions.
> OK
> >> +	{"o", &tcp_api, NULL},
> > This should be rather named as use_fastopen beacuse as it is, the name
> > is misleading (I was wondering if there were some changes in the TCP
> > userspace API for a while).
> There are changes in user-space TCP API: we don't use normal TCP system 
> calls: connect(), send(), write(). Instead making use of sendmsg(), 
> sendto() which are normally used in UDP. Some additional flags were 
> introduced.

Still I think it would be better to use 'fastopen' string in the opt
name.

> >> +int sock_recv_poll(int *fd, char *buf, const int *buf_size, int *offset)
> >> +{
> >> +	struct pollfd pfd;
> >> +	pfd.fd = *fd;
> >> +	pfd.events = POLLIN;
> >> +	int len = -1;
> >> +	while (1) {
> >> +		errno = 0;
> >> +		int ret = poll(&pfd, 1, wait_timeout / 1000);
> >> +		if (ret == -1) {
> >> +			if (errno == EINTR)
> >> +				continue;
> >> +			tst_resm(TFAIL | TERRNO, "poll failed at %s:%d",
> >> +				__FILE__, __LINE__);
> >> +			break;
> >> +		}
> >> +		if (ret == 0) {
> >> +			tst_resm(TFAIL, "msg timeout, sock '%d'", *fd);
> >> +			break;
> >> +		}
> >> +
> >> +		if (ret != 1 || !(pfd.revents & POLLIN))
> >> +			break;
> >> +
> >> +		errno = 0;
> >> +		len = recv(*fd, buf + *offset,
> >> +			*buf_size - *offset, MSG_DONTWAIT);
> >> +
> >> +		if (len == -1 && (errno == EINTR ||
> >> +			errno == EWOULDBLOCK || errno == EAGAIN))
> >> +			continue;
> >> +		else
> >> +			break;
> >> +	}
> >> +
> >> +	if (len == 0)
> >> +		tst_resm(TINFO, "sock was closed '%d'", *fd);
> >> +
> >> +	return len;
> >> +}
> > Now this one seems wrong. Both client and server creates new thread for
> > each connection, which means that this creates bussy loop (which will
> > eventually degrade performance). Why don't you use blocking read() so
> > that each thread waits in read() syscall util data are ready?
> It's not busy loop here, poll() will block. The purpose of the while(1) 
> is to go once more if the poll call was interrupted. I think I need to 
> remove EWOULDBLOCK and EGAIN errors checking as poll doesn't have their. 
> I didn't use read with block because I wanted some sort of block wait 
> timeout, which poll has.

Ah my bad, I see it now.

> > Can we default to localhost, would that work?
> It will work, but we won't see some significant performance results.

Ok.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
November Webinars for C, C++, Fortran Developers
Accelerate application performance with scalable programming models. Explore
techniques for threading, error checking, porting, and tuning. Get the most 
from the latest Intel processors and coprocessors. See abstracts and register
http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list