[ovs-dev] [optimize 08/13] netlink-socket: New function nl_sock_transact_multiple().

Ben Pfaff blp at nicira.com
Fri Oct 14 20:51:30 UTC 2011


On Thu, Oct 13, 2011 at 01:23:54PM -0700, Ethan Jackson wrote:
> I don't know a great deal about IOV maximums so the next comment may not be
> reasonable:
> I found _XOPEN_IOV_MAX a surprising minimum.  Wouldn't sysconf's opinion of
> max_iovs be less than _XOPEN_IOV_MAX in rare error conditions?

_XOPEN_IOV_MAX (which is always defined as 16) is the minimum value
that POSIX allows for IOV_MAX, so a system that supports fewer iovecs
than that is nonconformant.

> In this case wouldn't 1 be a safer minimum choice?

A kernel that supports only 1 iovec isn't very useful.  Anyway Linux
supports up to 1024 iovecs and this is a Linux-specific file, so maybe
I shouldn't have bothered with all this standards crap.

> +        } else if (max_iovs > MAX_IOVS) {
> +            max_iovs = 128;
> 
> 
> I think you want max_iovs = MAX_IOVS.

Yes, thanks.

> 
> +    max_batch_count = MAX(sock->rcvbuf / 4096, 1);
> +    max_batch_count = MIN(max_batch_count, MAX_IOVS / 2);
> 
> 
> Did you choose 4096 here because it's the typical page size?  Would it make
> sense to use the PAGESIZE macro if defined?

No, I chose it as a reasonable compromise between the maximum reply
size of 64 kB and the common reply size (about 0 bytes) that allows a
reasonable amount of batching with the typical rcvbuf size (about 128
kB)--see the comment.

> +        if (error == ENOBUFS) {
> +            VLOG_DBG_RL(&rl, "receive buffer overflow, resending request");
> 
> 
> Is this code supposed to resend the request itself, or is the caller
> responsible for handling that?

It automatically resends it in the next loop iteration.

> +        } else if (error) {
> +            VLOG_ERR_RL(&rl, "transaction error (%s)", strerror(error));
> +            nl_sock_record_errors__(transactions, n, error);
> +        }
> +    }
> +}
> 
> We may want to undef MAX_BATCH_BYTES.  Probably doesn't matter, but seems
> stylistically cleaner.

I changed it to an enum, thanks.



More information about the dev mailing list