Re: [bpf-next PATCH v3 2/7] bpf: add sendmsg option for testing BPF programs

Martin KaFai Lau
2018-01-13T07:30:58Z

On Thu, Jan 11, 2018 at 09:06:34PM -0800, John Fastabend wrote:
> When testing BPF programs using sockmap I often want to have more
> control over how sendmsg is exercised. This becomes even more useful
> as new sockmap program types are added.
>
> This adds a test type option to select type of test to run. Currently,
> only "ping" and "sendmsg" are supported, but more can be added as
> needed.
>
> The new help argument gives the following,
>
> Usage: ./sockmap --cgroup <cgroup_path>
> options:
> --help -h
> --cgroup -c
> --rate -r
> --verbose -v
> --iov_count -i
> --length -l
> --test -t
>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
> samples/sockmap/sockmap_user.c | 147 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 144 insertions(+), 3 deletions(-)
>
> diff --git a/samples/sockmap/sockmap_user.c b/samples/sockmap/sockmap_user.c
> index 17400d4..8ec7dbf 100644
> --- a/samples/sockmap/sockmap_user.c
> +++ b/samples/sockmap/sockmap_user.c
> @@ -56,6 +56,9 @@
> {"cgroup", required_argument, NULL, 'c' },
> {"rate", required_argument, NULL, 'r' },
> {"verbose", no_argument, NULL, 'v' },
> + {"iov_count", required_argument, NULL, 'i' },
> + {"length", required_argument, NULL, 'l' },
> + {"test", required_argument, NULL, 't' },
> {0, 0, NULL, 0 }
> };
>
> @@ -182,6 +185,117 @@ static int sockmap_init_sockets(void)
> return 0;
> }
>
> +struct msg_stats {
> + size_t bytes_sent;
> + size_t bytes_recvd;
> +};
> +
> +static int msg_loop(int fd, int iov_count, int iov_length, int cnt,
> + struct msg_stats *s, bool tx)
> +{
> + struct msghdr msg = {0};
> + struct iovec *iov;
> + int i, flags = 0;
> +
> + iov = calloc(iov_count, sizeof(struct iovec));
> + if (!iov)
> + return -ENOMEM;
I think errno has already been set to ENOMEM (instead of
-ENOMEM), so may directly use it instead.

> +
> + for (i = 0; i < iov_count; i++) {
> + char *d = calloc(iov_length, sizeof(char));
> +
> + if (!d) {
> + fprintf(stderr, "iov_count %i/%i OOM\n", i, iov_count);
> + free(iov);
> + return -ENOMEM;
The new "out_errno" label below should include freeing
all iov[i].iov_base.

Also, instead of a return here, I think you meant a "goto out_errno;"
such that the earlier "calloc(iov_length, sizeof(char));"
can also be freed during error.

Same for the following error return/goto cases

> + }
> + iov[i].iov_base = d;
> + iov[i].iov_len = iov_length;
> + }
> +
> + msg.msg_iov = iov;
> + msg.msg_iovlen = iov_count;
> +
> + if (tx) {
> + for (i = 0; i < cnt; i++) {
> + int sent = sendmsg(fd, &msg, flags);
> +
> + if (sent < 0) {
> + perror("send loop error:");
> + free(iov);
> + return sent;
> + }
> + s->bytes_sent += sent;
> + }
> + } else {
> + int slct, recv, max_fd = fd;
> + struct timeval timeout;
> + float total_bytes;
> + fd_set w;
> +
> + total_bytes = (float)iov_count * (float)iov_length * (float)cnt;
> + while (s->bytes_recvd < total_bytes) {
> + timeout.tv_sec = 1;
> + timeout.tv_usec = 0;
> +
> + /* FD sets */
> + FD_ZERO(&w);
> + FD_SET(fd, &w);
> +
> + slct = select(max_fd + 1, &w, NULL, NULL, &timeout);
> + if (slct == -1) {
> + perror("select()");
> + goto out_errno;
> + } else if (!slct) {
> + fprintf(stderr, "unexpected timeout\n");
> + goto out_errno;
Just in case, I think errno == 0 here here.

> + }
> +
> + recv = recvmsg(fd, &msg, flags);
> + if (recv < 0) {
> + if (errno != EWOULDBLOCK) {
> + perror("recv failed()\n");
> + goto out_errno;
> + }
> + }
> +
> + s->bytes_recvd += recv;
> + }
> + }
> +
> + for (i = 0; i < iov_count; i++)
> + free(iov[i].iov_base);
> + free(iov);
> + return 0;
> +out_errno:
> + free(iov);
> + return errno;
> +}
> +
> +static int sendmsg_test(int iov_count, int iov_buf, int cnt, int verbose)
> +{
> + struct msg_stats s = {0};
> + int err;
> +
> + err = msg_loop(c1, iov_count, iov_buf, cnt, &s, true);
> + if (err) {
> + fprintf(stderr,
> + "msg_loop_tx: iov_count %i iov_buf %i cnt %i err %i\n",
> + iov_count, iov_buf, cnt, err);
> + return err;
> + }
> +
> + msg_loop(p2, iov_count, iov_buf, cnt, &s, false);
err = msg_loop(...);

> + if (err)
> + fprintf(stderr,
> + "msg_loop_rx: iov_count %i iov_buf %i cnt %i err %i\n",
> + iov_count, iov_buf, cnt, err);
> +
> + fprintf(stdout, "sendmsg: TX_bytes %zu RX_bytes %zu\n",
> + s.bytes_sent, s.bytes_recvd);
> + return err;
> +}
> +
> static int forever_ping_pong(int rate, int verbose)
> {
> struct timeval timeout;
> @@ -257,13 +371,19 @@ static int forever_ping_pong(int rate, int verbose)
> return 0;
> }
>
> +enum {
> + PING_PONG,
> + SENDMSG,
> +};
> +
> int main(int argc, char **argv)
> {
> - int rate = 1, verbose = 0;
> + int iov_count = 1, length = 1024, rate = 1, verbose = 0;
> int opt, longindex, err, cg_fd = 0;
> + int test = PING_PONG;
> char filename[256];
>
> - while ((opt = getopt_long(argc, argv, "hvc:r:",
> + while ((opt = getopt_long(argc, argv, "hvc:r:i:l:t:",
> long_options, &longindex)) != -1) {
> switch (opt) {
> /* Cgroup configuration */
> @@ -282,6 +402,22 @@ int main(int argc, char **argv)
> case 'v':
> verbose = 1;
> break;
> + case 'i':
> + iov_count = atoi(optarg);
> + break;
> + case 'l':
> + length = atoi(optarg);
> + break;
> + case 't':
> + if (memcmp(optarg, "ping", 4) == 0) {
strcmp() may feel safer here just in case optarg could be shorter.

> + test = PING_PONG;
> + } else if (memcmp(optarg, "sendmsg", 7) == 0) {
Same here.

> + test = SENDMSG;
> + } else {
> + usage(argv);
> + return -1;
> + }
> + break;
> case 'h':
> default:
> usage(argv);
> @@ -339,7 +475,12 @@ int main(int argc, char **argv)
> goto out;
> }
>
> - err = forever_ping_pong(rate, verbose);
> + if (test == PING_PONG)
> + err = forever_ping_pong(rate, verbose);
> + else if (test == SENDMSG)
> + err = sendmsg_test(iov_count, length, rate, verbose);
> + else
> + fprintf(stderr, "unknown test\n");
> out:
> close(s1);
> close(s2);
>