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]

Date:   Wed, 14 Feb 2018 17:26:33 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Santosh Shilimkar <santosh.shilimkar@...cle.com>
Cc:     Sowmini Varadhan <sowmini.varadhan@...cle.com>,
        Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>, rds-devel@....oracle.com
Subject: Re: [PATCH V2 net-next 4/7] rds: support for zcopy completion notification

On Wed, Feb 14, 2018 at 1:50 PM, Santosh Shilimkar
<santosh.shilimkar@...cle.com> wrote:
> 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"

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.