From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3B4B9C43381 for ; Tue, 5 Mar 2019 14:24:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0B062206DD for ; Tue, 5 Mar 2019 14:24:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728365AbfCEOYm (ORCPT ); Tue, 5 Mar 2019 09:24:42 -0500 Received: from relay.sw.ru ([185.231.240.75]:58750 "EHLO relay.sw.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727940AbfCEOYl (ORCPT ); Tue, 5 Mar 2019 09:24:41 -0500 Received: from [172.16.24.21] by relay.sw.ru with esmtp (Exim 4.91) (envelope-from ) id 1h1Azl-0005ir-UV; Tue, 05 Mar 2019 17:24:38 +0300 Subject: Re: [PATCH] tcp: detect use sendpage for slab-based objects To: Eric Dumazet , Eric Dumazet Cc: netdev , Al Viro References: <4a7d3903-971d-26ea-1d70-514abff88f91@virtuozzo.com> From: Vasily Averin Message-ID: <85b6a52f-f997-6fc6-bc5a-99aaeaa27cc5@virtuozzo.com> Date: Tue, 5 Mar 2019 17:24:37 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 3/4/19 6:51 PM, Eric Dumazet wrote: > On 03/04/2019 04:58 AM, Vasily Averin wrote: >> Eric, what do you think about following patch? >> I validate its backported version on RHEL7 based OpenVZ kernel before sending to mainline. >> >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> index cf3c5095c10e..7be7b6abe8b5 100644 >> --- a/net/ipv4/tcp.c >> +++ b/net/ipv4/tcp.c >> @@ -943,6 +943,11 @@ ssize_t do_tcp_sendpages(struct sock *sk, struct page *page, int offset, >> ssize_t copied; >> long timeo = sock_sndtimeo(sk, flags & MSG_DONTWAIT); >> >> + if (PageSlab(page)) { >> + VM_WARN_ONCE(true, "sendpage should not handle Slab objects," >> + " please fix callers\n"); >> + return sock_no_sendpage_locked(sk, page, offset, size, flags); >> + } >> /* Wait for a connection to finish. One exception is TCP Fast Open >> * (passive side) where data is allowed to be sent before a connection >> * is fully established. >> > > There are at least four problems with this approach : > > 1) VM_WARN_ONCE() might be a NOP, and if not, it is simply some lines in syslog, > among thousands. > > 2) Falling back will give no incentive for callers to fix their code. We can return error instead of fallback, but yes, it means an extra (almost unneeded) check in TCP code. > 3) slowing down TCP, just because of some weird kernel-users. > I agree to add sanity check for everything user space can think of (aka syzbot), > but kernel users need to be fixed, without adding code in TCP. Do you advise to add PageSlab check into all .sendpage / .sendpacge_locked / tcp_sendpage / do_tcp_sednpages callers instead? > 4) sendpage() API is providing one page at a time. > We therefore call very expensive lock_sock() and release_sock() for every page. > sendfile() is sub optimal (compared to sendmsg(MSG_ZEROCOPY)) > There is an effort to provide batches of pages per round. > Your patch would cancel this effort, or make it very complicated.