Re: [PATCH V2 net-next 4/7] rds: support for zcopy completion notification

On Wed, Feb 14, 2018 at 1:50 PM, Santosh Shilimkar
[off-list ref] wrote:

On 2/14/2018 2:28 AM, Sowmini Varadhan wrote:

quoted

RDS removes a datagram (rds_message) 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.
When all references to the rds_message are quiesced, rds_message_purge
is called to release resources used by the rds_message

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.

rds_rm_zerocopy_callback() attempts to batch the number of cookies
sent with each notification  to a max of SO_EE_ORIGIN_MAX_ZCOOKIES.
This is achieved by checking the tail skb in the sk_error_queue:
if this has room for one more cookie, the cookie from the
current notification is added; else a new skb is added to the
sk_error_queue. Every invocation of rds_rm_zerocopy_callback() will
trigger a ->sk_error_report to notify the application.

Signed-off-by: Sowmini Varadhan <redacted>
---
v2:
   - make sure to always sock_put m_rs even if there is no znotifier.
   - major rewrite of notification, resulting in much simplification.

  include/uapi/linux/errqueue.h |    2 +
  net/rds/af_rds.c              |    2 +
  net/rds/message.c             |   83
+++++++++++++++++++++++++++++++++++++---
  net/rds/rds.h                 |   14 +++++++
  net/rds/recv.c                |    2 +
  5 files changed, 96 insertions(+), 7 deletions(-)

generic comment and please update it where it is applicable
in terms of variable names, notifiers etc.

RDS support true zero copy already with RDMA transport so some of
this code can easily get confused.

So I suggest something like below.
s/zerocopy/zeromsgcopy
s/zcopy/zmsgcopy
s/zcookie/zmsgcpycookie
s/znotifier/zmsgcpynotifier

quoted

diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h
index dc64cfa..28812ed 100644
--- a/include/uapi/linux/errqueue.h
+++ b/include/uapi/linux/errqueue.h

@@ -20,11 +20,13 @@ struct sock_extended_err {
  #define SO_EE_ORIGIN_ICMP6    3
  #define SO_EE_ORIGIN_TXSTATUS 4
  #define SO_EE_ORIGIN_ZEROCOPY 5
+#define SO_EE_ORIGIN_ZCOOKIE   6
  #define SO_EE_ORIGIN_TIMESTAMPING SO_EE_ORIGIN_TXSTATUS
    #define SO_EE_OFFENDER(ee)  ((struct sockaddr*)((ee)+1))
    #define SO_EE_CODE_ZEROCOPY_COPIED  1
+#define        SO_EE_ORIGIN_MAX_ZCOOKIES       8

This error change might need to go though other subsystem tree. May
be you can seperate it and also copy "linux-api@vger.kernel.org"

Previous changes to this file also went in through net-next, so this is
fine. The error queue is a socket, so network, datastructure.

As for naming, zerocopy is unfortunately an overused term. I did not
help matters when adding msg_zerocopy. That said, let's try to keep
consistent naming across socket families, so no zmsg prefix only in
RDS.