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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <331448b2-65d1-0f62-033c-b97d87a57b87@oracle.com>
Date:   Wed, 14 Feb 2018 10:50:32 -0800
From:   Santosh Shilimkar <santosh.shilimkar@...cle.com>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>,
        netdev@...r.kernel.org, willemdebruijn.kernel@...il.com
Cc:     davem@...emloft.net, rds-devel@....oracle.com
Subject: Re: [PATCH V2 net-next 4/7] rds: support for zcopy completion
 notification

On 2/14/2018 2:28 AM, Sowmini Varadhan wrote:
> 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 <sowmini.varadhan@...cle.com>
> ---
> 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	


> 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@...r.kernel.org"

Otherwise changes looks fine to me.

Regards,
Santosh