From mboxrd@z Thu Jan 1 00:00:00 1970 From: Santosh Shilimkar Subject: Re: [PATCH V2 net-next 5/7] rds: zerocopy Tx support. Date: Wed, 14 Feb 2018 13:14:08 -0800 Message-ID: References: <97bec2535a23b8dc976f958dc27256731de9127e.1517843755.git.sowmini.varadhan@oracle.com> <35efc079-0992-3ac7-ba8d-fcf021f7567f@oracle.com> <20180214194939.GO11528@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, willemdebruijn.kernel@gmail.com, davem@davemloft.net, rds-devel@oss.oracle.com To: Sowmini Varadhan Return-path: Received: from userp2130.oracle.com ([156.151.31.86]:41328 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030501AbeBNVOP (ORCPT ); Wed, 14 Feb 2018 16:14:15 -0500 In-Reply-To: <20180214194939.GO11528@oracle.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 2/14/2018 11:49 AM, Sowmini Varadhan wrote: > On (02/14/18 11:10), Santosh Shilimkar wrote: >> s/RDS_CMSG_ZCOPY_COOKIE/RDS_CMSG_ZMSGCOPY_COOKIE >> > > Please see https://www.spinics.net/lists/netdev/msg483627.html > Just saw it and responded to Dave. >>> @@ -356,6 +358,53 @@ int rds_message_copy_from_user(struct rds_message *rm, struct iov_iter *from) >>> sg = rm->data.op_sg; >>> sg_off = 0; /* Dear gcc, sg->page will be null from kzalloc. */ >>> + if (zcopy) { >>> + int total_copied = 0; >>> + struct sk_buff *skb; >>> + >>> + skb = alloc_skb(SO_EE_ORIGIN_MAX_ZCOOKIES * sizeof(u32), >>> + GFP_KERNEL); >> This can sleep so you might want to check if you want to use ATOMIC version >> here. > > I think it should be fine: rds_message_copy_from_user() is called > in process context, and if you notice, the calling function rds_sendmsg() > also has this > 1100 rm = rds_message_alloc(ret, GFP_KERNEL); > 1101 if (!rm) { > 1102 ret = -ENOMEM; > 1103 goto out; > 1104 } > > : > 1106 /* Attach data to the rm */ > : > 1113 ret = rds_message_copy_from_user(rm, &msg->msg_iter); > > So using GFP_KERNEL is as safe as the call at line 1100. > Was just asking you to check if it is safe. The path already does that so we are good. > >>> + return -ENOMEM; >>> + } >> NOMEM new application visible change but probably the right one for this >> particular case. Just need to make sure application can handle this >> error. > > I think the application already handles this correctly (see line 1102 above) > Indeed. Thanks for checking. Regards, Santosh