rds: zerocopy support

This patch series provides support for MSG_ZERCOCOPY on a PF_RDS socket based on the APIs and infrastructure added by f214f915e7db ("tcp: enable MSG_ZEROCOPY")

For single threaded rds-stress testing using rds-tcp with the ixgbe driver using 1M message sizes (-a 1M -q 1M) preliminary results show that there is a significant reduction in latency: about 90 usec with zerocopy, compared with 200 usec without zerocopy.

Additional testing/debugging is ongoing, but I am sharing the current patchset to get some feedback on API design choices especially for the send-completion notification for multi-threaded datagram socket applications

Brief RDS Architectural overview: PF_RDS sockets implement message-bounded datagram semantics over a reliable transport. The RDS socket layer tracks message boundaries and uses an underlying transport like TCP to segment/reassemble the message into MTU sized frames. In addition to the reliable, ordered delivery semantics provided by the transport, the RDS layer also retains the datagram in its retransmit queue, to be resent in case of transport failure/restart events.

This patchset modifies the above for zerocopy in the following manner.
- if the MSG_ZEROCOPY flag is specified with rds_sendmsg(), and,
- if the SO_ZEROCOPY socket option has been set on the PF_RDS socket, application pages sent down with rds_sendmsg are pinned. The pinning uses the accounting infrastructure added by a91dbff551a6 ("sock: ulimit on MSG_ZEROCOPY pages")

The message is unpinned after we get back an ACK (TCP ACK, in the case of rds-tcp) indicating that the RDS module at the receiver has received the datagram, and it is safe for the sender to free the message from its (RDS) retransmit queue.

The payload bytes in the message may not be modified for the duration that the message has been pinned. A multi-threaded application using this infrastructure thus needs to be notified about send-completion, and that notification must uniquely identify the message to the application so that the application buffers may be freed/reused.

Unique identification of the message in the completion notification is done in the following manner:
- application passes down a 32 bit cookie as ancillary data with rds_sendmsg. The ancillary data in this case has cmsg_level == SOL_RDS and cmsg_type == RDS_CMSG_ZCOPY_COOKIE.
- upon send-completion, the rds module passes up a batch of cookies on the sk_error_queue associated with the PF_RDS socket. The message thus received will have a batch of N cookies in the data, with the number of cookies (N) specified in the ancillary data passed with recvmsg(). The current patchset sets up the ancillary data as a sock_extended_err with ee_origin == SO_EE_ORIGIN_ZEROCOPY, and ee_data == N based on 52267790ef52 ("sock: add MSG_ZEROCOPY"), and alternate suggestions for designing this API are invited. The important point here is that the notification would need to be able to contain an arbitrary number of cookies, where each cookie would allow the application to uniquely identify a buffer used with sendmsg()

Note that cookie-batching on send-completion notification means that the application may not know the buffering requirements a priori and the buffer sent down with recvmsg on the MSG_ERRQUEUE may be smaller than the required size for the notifications to be sent. To accomodate this case, sk_error_queue has been enhanced to support MSG_PEEK semantics (so that the application can retry with a larger buffer)

Work in progress
- additional testing: when we test this with rds-stress with 8 sockets, and a send depth of 64 (i.e. each socket can have at most 64 outstanding requests) some data corruption is reported by rds-stress. Working on drilling down the root-cause
- optimizing the send-completion notification API: our use-cases are multi-threaded, and we want to be able to reuse buffers as soon as possible (instead of waiting for the req-resp transaction to complete). Sub-optimal design of the completion notification can actually cause a perf deterioration (system-call overhead to reap notification, throughput can go down because application does not send "fast enough", even though latency is small), so this area needs to be optimized carefully
- additional test results beyond the rds-stress micro-benchmarks.

Sowmini Varadhan (6):
sock: MSG_PEEK support for sk_error_queue
skbuff: export mm_[un]account_pinned_pages for other modules
rds: hold a sock ref from rds_message to the rds_sock
sock: permit SO_ZEROCOPY on PF_RDS socket
rds: support for zcopy completion notification
rds: zerocopy Tx support.

 drivers/net/tun.c        |    2 +-
 include/linux/skbuff.h   |    3 +
 include/net/sock.h       |    2 +-
 include/uapi/linux/rds.h |    1 +
 net/core/skbuff.c        |    6 ++-
 net/core/sock.c          |   14 +++++-
 net/packet/af_packet.c   |    3 +-
 net/rds/af_rds.c         |    3 +
 net/rds/message.c        |  119 ++++++++++++++++++++++++++++++++++++++++++++-
 net/rds/rds.h            |   16 +++++-
 net/rds/recv.c           |    3 +
 net/rds/send.c           |   41 ++++++++++++----
 12 files changed, 192 insertions(+), 21 deletions(-)

Allow the application the ability to use MSG_PEEK with sk_error_queue so that it can peek and re-read message in cases where MSG_TRUNC may be encountered.

---
 drivers/net/tun.c      |    2 +-
 include/net/sock.h     |    2 +-
 net/core/sock.c        |    7 +++++--
 net/packet/af_packet.c |    3 ++-
 4 files changed, 9 insertions(+), 5 deletions(-)

index 2fba3be..cfd0e0f 100644
@@ -2313,7 +2313,7 @@ static int tun_recvmsg(struct socket *sock, struct msghdr *m, size_t total_len,
 	}
 	if (flags & MSG_ERRQUEUE) {
 		ret = sock_recv_errqueue(sock->sk, m, total_len,
-					 SOL_PACKET, TUN_TX_TIMESTAMP);
+					 SOL_PACKET, TUN_TX_TIMESTAMP, flags);
 		goto out;
 	}
 	ret = tun_do_read(tun, tfile, &m->msg_iter, flags & MSG_DONTWAIT, ptr);

index 73b7830..f0b6990 100644
@@ -2343,7 +2343,7 @@ static inline bool sk_listener(const struct sock *sk)
 int sock_get_timestamp(struct sock *, struct timeval __user *);
 int sock_get_timestampns(struct sock *, struct timespec __user *);
 int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len, int level,
-		       int type);
+		       int type, int flags);
 
 bool sk_ns_capable(const struct sock *sk,
 		   struct user_namespace *user_ns, int cap);

index 72d14b2..4f52677 100644
@@ -2887,7 +2887,7 @@ void sock_enable_timestamp(struct sock *sk, int flag)
 }
 
 int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len,
-		       int level, int type)
+		       int level, int type, int flags)
 {
 	struct sock_exterr_skb *serr;
 	struct sk_buff *skb;

@@ -2916,7 +2916,10 @@ int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len,
 	err = copied;
 
 out_free_skb:
-	kfree_skb(skb);
+	if (likely(!(flags & MSG_PEEK)))
+		kfree_skb(skb);
+	else
+		skb_queue_head(&sk->sk_error_queue, skb);
 out:
 	return err;
 }

index ee7aa0b..4314f31 100644
@@ -3294,7 +3294,8 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
 
 	if (flags & MSG_ERRQUEUE) {
 		err = sock_recv_errqueue(sk, msg, len,
-					 SOL_PACKET, PACKET_TX_TIMESTAMP);
+					 SOL_PACKET, PACKET_TX_TIMESTAMP,
+					 flags);
 		goto out;
 	}
 

1.7.1

Yes, I did notice the potential for re-ordering when writing the patch.. but these are not actuallly messages from the wire, so is re-ordering fatal?

In general, I"m not particularly attached to this solution- in my testing, I'm seeing that it's possible to reduce the latency and still take a hit on the throughput if the application does not reap the completion notifciation (and send out new data) efficiently

Some (radically differnt) alternatives that were suggested to me

- send up all the cookies as ancillary data with recvmsg (i.e., send it as a cmsgdata along with actual data from the wire). In most cases, the application has data to read, anyway. If it doesnt (pure sender), we could wake up recvmsg with 0 bytes of data, but with the cookie info in the ancillary data. This feels not-so-elegant to me, but I suppose it would have the benefit of optimizing on the syscall overhead.. (and you could use MSG_CTRUNC to handle the case of insuufficient bufffer for cookies, sending the rest on the next call)..
- allow application to use a setsockopt on the rds socket, with some shmem region, into which the kernel could write the cookies, Let application reap cookies without syscall overhead from that shmem region..

This may be "good enough" as well, maybe allow a max of (say) 16 cookies, and set up the skb's in the error queue to send up batches of 16 cookies at a time?

--Sowmini

Some applications out there would break horribly, trust me.

so I'm not particularly attached to that solution, and I appreciate the wisdom (and the NACK), but lets try to find a useful alternative

The current zcopy completion notification mechanism involves syscall overhead already, and is also inadequate for threaded applications sharing an fd. Plus it wont work for datagram sockets.

I'm fine with Willem's suggestion of passing a fixed number of cookies as ancillary data (with CTRUNC to denote inadequate buffer) but if we are really so thrifty about syscall overhead, we should not be using sk_error_queue in the first place- perhaps we can pass up the completion notification as ancillary data with recvmsg() on the POLLIN channel itself (which is weird if there is no data to recv, and only ancillary info to pass up, but hey, we are "performant"!).

--Sowmini

The thing is : MSG_PEEK 'support' will also need SO_PEEK_OFF support.

And begins the crazy stuff.

So lets properly design things, and not re-use legacy stuff that is proven to be not multi-thread ready and too complex.

If you want to design a new channel of communication, do it, and maintain it.

sure, I'll drop the MSG_PEEK idea (which I wasnt very thrilled about anyway)

My instinct is to go with the fixed size ancillary data- which itself allows 2 options:

1. cmsg_data has a sock_extended_err preamble
with ee_origin = SO_EE_ORIGIN_ZEROCOPY_COOKIE (or similar), and the ee_data is an array of 32 bit cookies (can pack at most 8 32-bit cookies, if we want to pack this into an skb->cb)

Using the sock_extended_err as preamble will allow this to be usable by existing tcp zcopy applications (they can use the ee_origin to find out if this a batch of cookies or the existing hi/lo values).

2. If we have the option of passing completion-notification up as ancillary data on the pollin/recvmsg channel itself (instead of MSG_ERRQUEUE) we dont have to try to retain "backward compat" to the SO_EE_ORIGIN_ZEROCOPY API: we can just use a completely new data struct for the notification and potentially pack more cookies into 48 bytes (RDS could be the first guinea pig for this- doesnt even have to be done across all protocol families on day-1).

I think the shmem channel suggestion would be an optional optimization that can be added later- it may not even be necessary, since most applications will likely be sending *and* receiving data, so passing up cookies with recvmsg should be "good enough" to save syscall overhead for the common case.

I can work #2, if there are no objections to it.

--Sowmini

This assumes a somewhat symmetric workload, where there are enough recv calls to reap the notification associated with the send calls.

I would stay with MSG_ERRQUEUE processing. One option is to pass data up to userspace in the data portion of the notification skb instead of encoding it in ancillary data, like tcp_get_timestamping_opt_stats.

Your comment about the assumption is true, but at least for the database use-cases, we have a request-response model, so the assumption works out.. I dont know if many other workloads that send large buffers have this pattern.

that's similar to what I have, except that it does not have the MSG_PEEK part (you'd need to enforce that the data portion is upper-bounded, and that the application has the responsibility of sending down "enough" buffer with recvmsg).

Note that any one of these choices are ok with me- I have no special attachments to any of them.

--Sowmini

If that is true in general for PF_RDS, then it is a reasonable approach. How about treating it as a (follow-on) optimization path. Opportunistic piggybacking of notifications on data reads is more widely applicable.

Right. I think that an upper bound is the simplest solution here.

By the way, if you allocate an skb immediately on page pinning, then there are always sufficient skbs to store all notifications. On errqueue enqueue just drop the new skb and copy its notification to the body of the skb already on the queue, if one exists and it has room. That is essentially what the tcp zerocopy code does with the [data, info] range.

sounds good.

ok, I'll give that a shot (I'm working through the other review comments as well)

fwiw, the data-corruption issue I mentioned turned out to be a day-one bug in rds-tcp (patched in http://patchwork.ozlabs.org/patch/863183/).
The buffer reaping with zcopy (and aggressiveness of rds-stress) brought this one out..

--Sowmini

Thanks. Good to hear that it's not in zerocopy, itself.

Lets not add buggy feature that only fuzzers love to use to trigger awful bugs.

No serious (performance sensitive) application use MSG_PEEK, because of extra syscall overhead.

RDS would like to use the helper functions for managing pinned pages added by Commit a91dbff551a6 ("sock: ulimit on MSG_ZEROCOPY pages")

---
 include/linux/skbuff.h |    3 +++
 net/core/skbuff.c      |    6 ++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

index b8e0da6..8e2730a 100644
@@ -466,6 +466,9 @@ struct ubuf_info {
 
 #define skb_uarg(SKB)	((struct ubuf_info *)(skb_shinfo(SKB)->destructor_arg))
 
+int mm_account_pinned_pages(struct mmpin *mmp, size_t size);
+void mm_unaccount_pinned_pages(struct mmpin *mmp);
+
 struct ubuf_info *sock_zerocopy_alloc(struct sock *sk, size_t size);
 struct ubuf_info *sock_zerocopy_realloc(struct sock *sk, size_t size,
 					struct ubuf_info *uarg);

index 01e8285..272a513 100644
@@ -890,7 +890,7 @@ struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src)
 }
 EXPORT_SYMBOL_GPL(skb_morph);
 
-static int mm_account_pinned_pages(struct mmpin *mmp, size_t size)
+int mm_account_pinned_pages(struct mmpin *mmp, size_t size)
 {
 	unsigned long max_pg, num_pg, new_pg, old_pg;
 	struct user_struct *user;

@@ -919,14 +919,16 @@ static int mm_account_pinned_pages(struct mmpin *mmp, size_t size)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(mm_account_pinned_pages);
 
-static void mm_unaccount_pinned_pages(struct mmpin *mmp)
+void mm_unaccount_pinned_pages(struct mmpin *mmp)
 {
 	if (mmp->user) {
 		atomic_long_sub(mmp->num_pg, &mmp->user->locked_vm);
 		free_uid(mmp->user);
 	}
 }
+EXPORT_SYMBOL_GPL(mm_unaccount_pinned_pages);
 
 struct ubuf_info *sock_zerocopy_alloc(struct sock *sk, size_t size)
 {

1.7.1

The existing model holds a reference from the rds_sock to the rds_message, but the rds_message does not itself hold a sock_put() on the rds_sock. Instead the m_rs field in the rds_message is assigned when the message is queued on the sock, and nulled when the message is dequeued from the sock.

We want to be able to notify userspace when the rds_message is actually freed (from rds_message_purge(), after the refcounts to the rds_message go to 0). These notifications will signal that it is safe for uspace to free/reuse any pages that may have been pinned down for zerocopy, and are sent up on the PF_RDS socket.

In order to be able to send the notifications on the rds_sock, we need to retain the m_rs assignment in the rds_message with the necessary refcount book-keeping.

---
 net/rds/message.c |    8 +++++++-
 net/rds/send.c    |    7 +------
 2 files changed, 8 insertions(+), 7 deletions(-)

index 4318cc9..ef3daaf 100644
@@ -58,7 +58,7 @@ void rds_message_addref(struct rds_message *rm)
  */
 static void rds_message_purge(struct rds_message *rm)
 {
-	unsigned long i;
+	unsigned long i, flags;
 
 	if (unlikely(test_bit(RDS_MSG_PAGEVEC, &rm->m_flags)))
 		return;

@@ -69,6 +69,12 @@ static void rds_message_purge(struct rds_message *rm)
 		__free_page(sg_page(&rm->data.op_sg[i]));
 	}
 	rm->data.op_nents = 0;
+	spin_lock_irqsave(&rm->m_rs_lock, flags);
+	if (rm->m_rs) {
+		sock_put(rds_rs_to_sk(rm->m_rs));
+		rm->m_rs = NULL;
+	}
+	spin_unlock_irqrestore(&rm->m_rs_lock, flags);
 
 	if (rm->rdma.op_active)
 		rds_rdma_free_op(&rm->rdma);

index d3e32d1..5ac0925 100644
@@ -649,7 +649,6 @@ static void rds_send_remove_from_sock(struct list_head *messages, int status)
 				rm->rdma.op_notifier = NULL;
 			}
 			was_on_sock = 1;
-			rm->m_rs = NULL;
 		}
 		spin_unlock(&rs->rs_lock);
 

@@ -756,9 +755,6 @@ void rds_send_drop_to(struct rds_sock *rs, struct sockaddr_in *dest)
 		 */
 		if (!test_and_clear_bit(RDS_MSG_ON_CONN, &rm->m_flags)) {
 			spin_unlock_irqrestore(&cp->cp_lock, flags);
-			spin_lock_irqsave(&rm->m_rs_lock, flags);
-			rm->m_rs = NULL;
-			spin_unlock_irqrestore(&rm->m_rs_lock, flags);
 			continue;
 		}
 		list_del_init(&rm->m_conn_item);

@@ -774,7 +770,6 @@ void rds_send_drop_to(struct rds_sock *rs, struct sockaddr_in *dest)
 		__rds_send_complete(rs, rm, RDS_RDMA_CANCELED);
 		spin_unlock(&rs->rs_lock);
 
-		rm->m_rs = NULL;
 		spin_unlock_irqrestore(&rm->m_rs_lock, flags);
 
 		rds_message_put(rm);

@@ -798,7 +793,6 @@ void rds_send_drop_to(struct rds_sock *rs, struct sockaddr_in *dest)
 		__rds_send_complete(rs, rm, RDS_RDMA_CANCELED);
 		spin_unlock(&rs->rs_lock);
 
-		rm->m_rs = NULL;
 		spin_unlock_irqrestore(&rm->m_rs_lock, flags);
 
 		rds_message_put(rm);

@@ -849,6 +843,7 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
 		list_add_tail(&rm->m_sock_item, &rs->rs_send_queue);
 		set_bit(RDS_MSG_ON_SOCK, &rm->m_flags);
 		rds_message_addref(rm);
+		sock_hold(rds_rs_to_sk(rs));
 		rm->m_rs = rs;
 
 		/* The code ordering is a little weird, but we're

1.7.1

allow the application to set SO_ZEROCOPY on the underlying sk of a PF_RDS socket

---
 net/core/sock.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

index 4f52677..f0f44b0 100644
@@ -1049,6 +1049,13 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case SO_ZEROCOPY:
+		if (sk->sk_family == PF_RDS) {
+			if (val < 0 || val > 1)
+				ret = -EINVAL;
+			else
+				sock_valbool_flag(sk, SOCK_ZEROCOPY, valbool);
+			break;
+		}
 		if (sk->sk_family != PF_INET && sk->sk_family != PF_INET6)
 			ret = -ENOTSUPP;
 		else if (sk->sk_protocol != IPPROTO_TCP)

1.7.1

RDS removes a datagram from the retransmit queue when an ACK is received. The ACK indicates that the receiver has queued the RDS datagram, so that the sender can safely forget the datagram.

If the datagram to be removed had pinned pages set up, add an entry to the rs->rs_znotify_queue so that the notifcation will be sent up via rds_rm_zerocopy_callback() when the rds_message is eventually freed by rds_message_purge.

---
 net/rds/af_rds.c  |    3 ++
 net/rds/message.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 net/rds/rds.h     |   13 +++++++++-
 net/rds/recv.c    |    3 ++
 net/rds/send.c    |    7 +++++
 5 files changed, 91 insertions(+), 2 deletions(-)

index b405f77..23126db 100644
@@ -183,6 +183,8 @@ static unsigned int rds_poll(struct file *file, struct socket *sock,
 		mask |= (POLLIN | POLLRDNORM);
 	if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))
 		mask |= (POLLOUT | POLLWRNORM);
+	if (sk->sk_err || !skb_queue_empty(&sk->sk_error_queue))
+		mask |= POLLERR;
 	read_unlock_irqrestore(&rs->rs_recv_lock, flags);
 
 	/* clear state any time we wake a seen-congested socket */

@@ -511,6 +513,7 @@ static int __rds_create(struct socket *sock, struct sock *sk, int protocol)
 	INIT_LIST_HEAD(&rs->rs_send_queue);
 	INIT_LIST_HEAD(&rs->rs_recv_queue);
 	INIT_LIST_HEAD(&rs->rs_notify_queue);
+	INIT_LIST_HEAD(&rs->rs_znotify_queue);
 	INIT_LIST_HEAD(&rs->rs_cong_list);
 	spin_lock_init(&rs->rs_rdma_lock);
 	rs->rs_rdma_keys = RB_ROOT;

index 374ae83..de5015a 100644
@@ -356,6 +356,12 @@ static inline u32 rds_rdma_cookie_offset(rds_rdma_cookie_t cookie)
 #define RDS_MSG_PAGEVEC		7
 #define RDS_MSG_FLUSH		8
 
+struct rds_znotifier {
+	struct list_head	z_list;
+	u32			z_cookie;
+	struct mmpin		z_mmp;
+};
+
 struct rds_message {
 	refcount_t		m_refcount;
 	struct list_head	m_sock_item;

@@ -431,11 +437,14 @@ struct rds_message {
 		} rdma;
 		struct rm_data_op {
 			unsigned int		op_active:1;
-			unsigned int		op_notify:1;
+			unsigned int		op_notify:1,
+						op_zcopy:1,
+						op_pad_to_32:30;
 			unsigned int		op_nents;
 			unsigned int		op_count;
 			unsigned int		op_dmasg;
 			unsigned int		op_dmaoff;
+			struct rds_znotifier	*op_mmp_znotifier;
 			struct scatterlist	*op_sg;
 		} data;
 	};

@@ -588,6 +597,8 @@ struct rds_sock {
 	/* Socket receive path trace points*/
 	u8			rs_rx_traces;
 	u8			rs_rx_trace[RDS_MSG_RX_DGRAM_TRACE_MAX];
+
+	struct list_head	rs_znotify_queue; /* zerocopy completion */
 };
 
 static inline struct rds_sock *rds_sk_to_rs(const struct sock *sk)

index b25bcfe..043f667 100644
@@ -594,6 +594,9 @@ int rds_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 
 	if (msg_flags & MSG_OOB)
 		goto out;
+	if (msg_flags & MSG_ERRQUEUE)
+		return sock_recv_errqueue(sk, msg, size, SOL_IP, IP_RECVERR,
+					  msg_flags);
 
 	while (1) {
 		/* If there are pending notifications, do those - and nothing else */

index 5ac0925..5c38ce3 100644
@@ -635,7 +635,14 @@ static void rds_send_remove_from_sock(struct list_head *messages, int status)
 		if (test_and_clear_bit(RDS_MSG_ON_SOCK, &rm->m_flags)) {
 			struct rm_rdma_op *ro = &rm->rdma;
 			struct rds_notifier *notifier;
+			struct rds_znotifier *znotifier;
 
+			if (rm->data.op_zcopy) {
+				znotifier = rm->data.op_mmp_znotifier;
+				list_add_tail(&znotifier->z_list,
+					      &rs->rs_znotify_queue);
+				rm->data.op_mmp_znotifier = NULL;
+			}
 			list_del_init(&rm->m_sock_item);
 			rds_send_sndbuf_remove(rs, rm);
 

1.7.1

     :

right, I could allocate the skb when we set up the zcopy data strucutres.

I would have liked to reuse skb->cb, but had to fall back to the alloc_skb because of the attempt to fall back to flexibly numbered (and sized) cookie notifications.

If we choose the "max number of cookies" option discussed in the previous thread, I think I should be able to do this with the existing 48 byte sized cb and pack in 8 32 bit cookies after the sock_extended_err.. maybe that's sufficient.

sounds good, I'll take care of this (and other review comments) for the next rev.

I'm not sure I understand the proposal- you want sendmsg to return the cookie ("zckey") for you? How?

even if we mangled sendmsg() to return something other than the existing POSIX semantics, how will the application be asynchronously notified that send has completed (on a per-message/per-datagram) basis?

not so. rds-stress [1] with -d8 -t8 quickly disproves this on my 10G ixgbe connection.

When you have multiple threads writing to a socket, you cannot know what was the "order of send", unless you bottleneck all the threads to go through a single-point-of-send. rds-stress is an example of this sort of usage (fairly typical in our applications)

[1] http://public-yum.oracle.com/repo/OracleLinux/OL6/ofed_UEK/x86_64//getPackageSource/rds-tools-2.0.7-1.12.el6.src.rpm

Consider 2 RDS sockets fd1 and fd2, each one sending to the
same pair of IP addresses: if fd1 gets bound to tcp sock1, fd2 to tcp sock2, it's very possible that the send completion is not in the same order as the order of send. The application cannot know which socket gets bound to which TCP connection (or even whether/how-many tcp connections are involved: mprds strives to make this transparent to the application)

Same problem exists for pure-datagram sockets like PF_PACKET, UDP etc. application may send buf1 (datagram1) and buf2 (datagram2), and buf2 may make it to the destination before buf1. The "notifications arrive largely in order" may be mostly true about a single-stream TCP connection but not for the datagram models (or even threaded tcp apps like iperf?)..

good point, I'll check into this and fix (same applies for the comments to patches 4/6 and 6/6)

                                                     :

To make sure I dont misunderstand, you are suggesting that we dont need op_zcopy, but can just check for the null-ness of op_mmp_znotifier (yes, true, I agree)? or something else?

--Sowmini

I'm purposely glossing over how the kernel returns this item for now. Was just wondering whether we can then assume mostly in order delivery and reuse the existing notification interface from tcp zerocopy.

there is little benefit to trying to force linear IDs derived from sk->sk_zckey.

Okay. In that case, the cmsg cookie approach sounds fine. I had the same in an early tcp zerocopy test version, as a matter of fact.

Yes, that's what I meant.

If the MSG_ZEROCOPY flag is specified with rds_sendmsg(), and, if the SO_ZEROCOPY socket option has been set on the PF_RDS socket, application pages sent down with rds_sendmsg() are pinned.

The pinning uses the accounting infrastructure added by Commit a91dbff551a6 ("sock: ulimit on MSG_ZEROCOPY pages")

The payload bytes in the message may not be modified for the duration that the message has been pinned. A multi-threaded application using this infrastructure may thus need to be notified about send-completion so that it can free/reuse the buffers passed to rds_sendmsg(). Notification of send-completion will identify each message-buffer by a cookie that the application must specify as ancillary data to rds_sendmsg().
The ancillary data in this case has cmsg_level == SOL_RDS and cmsg_type == RDS_CMSG_ZCOPY_COOKIE.

---
 include/uapi/linux/rds.h |    1 +
 net/rds/message.c        |   44 +++++++++++++++++++++++++++++++++++++++++++-
 net/rds/rds.h            |    3 ++-
 net/rds/send.c           |   27 ++++++++++++++++++++++++---
 4 files changed, 70 insertions(+), 5 deletions(-)

index e71d449..09b8cc6 100644
@@ -102,6 +102,7 @@
 #define RDS_CMSG_ATOMIC_CSWP		7
 #define RDS_CMSG_MASKED_ATOMIC_FADD	8
 #define RDS_CMSG_MASKED_ATOMIC_CSWP	9
+#define	RDS_CMSG_ZCOPY_COOKIE		10
 #define RDS_CMSG_RXPATH_LATENCY		11
 
 #define RDS_INFO_FIRST			10000

index de5015a..884d61c 100644
@@ -781,7 +781,8 @@ void rds_for_each_conn_info(struct socket *sock, unsigned int len,
 /* message.c */
 struct rds_message *rds_message_alloc(unsigned int nents, gfp_t gfp);
 struct scatterlist *rds_message_alloc_sgs(struct rds_message *rm, int nents);
-int rds_message_copy_from_user(struct rds_message *rm, struct iov_iter *from);
+int rds_message_copy_from_user(struct rds_message *rm, struct iov_iter *from,
+			       struct rds_sock *rs, bool zcopy);
 struct rds_message *rds_message_map_pages(unsigned long *page_addrs, unsigned int total_len);
 void rds_message_populate_header(struct rds_header *hdr, __be16 sport,
 				 __be16 dport, u64 seq);

index 5c38ce3..e085730 100644
@@ -908,6 +908,7 @@ static int rds_rm_size(struct msghdr *msg, int data_len)
 
 		case RDS_CMSG_RDMA_DEST:
 		case RDS_CMSG_RDMA_MAP:
+		case RDS_CMSG_ZCOPY_COOKIE:
 			cmsg_groups |= 2;
 			/* these are valid but do no add any size */
 			break;

@@ -935,6 +936,18 @@ static int rds_rm_size(struct msghdr *msg, int data_len)
 	return size;
 }
 
+static int rds_cmsg_zcopy(struct rds_sock *rs, struct rds_message *rm,
+			  struct cmsghdr *cmsg)
+{
+	unsigned int *cookie;
+
+	if (cmsg->cmsg_len < CMSG_LEN(sizeof(*cookie)))
+		return -EINVAL;
+	cookie = CMSG_DATA(cmsg);
+	rm->data.op_mmp_znotifier->z_cookie = *cookie;
+	return 0;
+}
+
 static int rds_cmsg_send(struct rds_sock *rs, struct rds_message *rm,
 			 struct msghdr *msg, int *allocated_mr)
 {

@@ -977,6 +990,10 @@ static int rds_cmsg_send(struct rds_sock *rs, struct rds_message *rm,
 			ret = rds_cmsg_atomic(rs, rm, cmsg);
 			break;
 
+		case RDS_CMSG_ZCOPY_COOKIE:
+			ret = rds_cmsg_zcopy(rs, rm, cmsg);
+			break;
+
 		default:
 			return -EINVAL;
 		}

@@ -1047,10 +1064,12 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 	long timeo = sock_sndtimeo(sk, nonblock);
 	struct rds_conn_path *cpath;
 	size_t total_payload_len = payload_len, rdma_payload_len = 0;
+	bool zcopy = ((msg->msg_flags & MSG_ZEROCOPY) &&
+		      sock_flag(rds_rs_to_sk(rs), SOCK_ZEROCOPY));
 
 	/* Mirror Linux UDP mirror of BSD error message compatibility */
 	/* XXX: Perhaps MSG_MORE someday */
-	if (msg->msg_flags & ~(MSG_DONTWAIT | MSG_CMSG_COMPAT)) {
+	if (msg->msg_flags & ~(MSG_DONTWAIT | MSG_CMSG_COMPAT | MSG_ZEROCOPY)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}

@@ -1107,12 +1126,14 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 
 	/* Attach data to the rm */
 	if (payload_len) {
-		rm->data.op_sg = rds_message_alloc_sgs(rm, ceil(payload_len, PAGE_SIZE));
+		int num_sgs = ceil(payload_len, PAGE_SIZE);
+
+		rm->data.op_sg = rds_message_alloc_sgs(rm, num_sgs);
 		if (!rm->data.op_sg) {
 			ret = -ENOMEM;
 			goto out;
 		}
-		ret = rds_message_copy_from_user(rm, &msg->msg_iter);
+		ret = rds_message_copy_from_user(rm, &msg->msg_iter, rs, zcopy);
 		if (ret)
 			goto out;
 	}

1.7.1