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

[<ch...@su...>]

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
ch...@su...