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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id C554BC4332F for ; Thu, 14 Dec 2023 02:27:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3EBA28D0089; Wed, 13 Dec 2023 21:27:06 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 39D198D0083; Wed, 13 Dec 2023 21:27:06 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 23EA68D0089; Wed, 13 Dec 2023 21:27:06 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 0F11D8D0083 for ; Wed, 13 Dec 2023 21:27:06 -0500 (EST) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id CA65B160BAF for ; Thu, 14 Dec 2023 02:27:05 +0000 (UTC) X-FDA: 81563836410.22.22F0E08 Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) by imf26.hostedemail.com (Postfix) with ESMTP id F1F33140018 for ; Thu, 14 Dec 2023 02:27:01 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ZKeMUYCy; spf=pass (imf26.hostedemail.com: domain of liangchen.linux@gmail.com designates 209.85.208.54 as permitted sender) smtp.mailfrom=liangchen.linux@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1702520822; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=BY8e/SFb9V+otPXNlhB6x6Lcjib+XLpc12tNaO8DAF0=; b=mPwwpYMPphocP0IWGJ2JmN33zeVGwGFPVU5OSB/kTTI3gqZCJ7dGo163gx9Tv5xgVWzwoL MTjrCuXm1l6d535X58xOPTcxRsKy9kTLo/uku6DpVIHHUtl9dpxP9S5NfI+AxLeuGsxheC 2QmcYAvkDoLwuX1w0msM909nSdzSZ4c= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=ZKeMUYCy; spf=pass (imf26.hostedemail.com: domain of liangchen.linux@gmail.com designates 209.85.208.54 as permitted sender) smtp.mailfrom=liangchen.linux@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702520822; a=rsa-sha256; cv=none; b=N7/EWCR+jp5KQqiyazuhjEfWpnN/2LvPP0/HOTWYeBocgs9wuL6fgdb8hcGith1zycBq5e KHKXjkIpKv+wA9OBYIo3/fBvhAMJqOcxojJjnECmk7BrI2nBC8zo59qzpZOqJkGP/hTspN /r6aq1uMaaKBFUoBp85i2cBPEuyolfo= Received: by mail-ed1-f54.google.com with SMTP id 4fb4d7f45d1cf-552750aae1eso102930a12.1 for ; Wed, 13 Dec 2023 18:27:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1702520820; x=1703125620; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=BY8e/SFb9V+otPXNlhB6x6Lcjib+XLpc12tNaO8DAF0=; b=ZKeMUYCyyFCbeo07e0l/BjfhCCulhCGROZqEqd8oFU5hMTN7Fc86AtHwHtT1yjOAWM xHdzLIqvROa+I+fG7DZAAxuXy655G7+m3V9pBDaGJ6hR7PHIl89SK/dZWzikkM2k20S4 T4AlRX4sts6ea9fP253FSXo3/ZPf/GOOp7VvTcxR3A/Q+z8xKmRlts4ZSPKqiGUMqMHJ ++1bE1jW+jpsk4wV2iJVLH5TZbwVkkmpOM8cKhvlKGNq6bvBhL2moUdrDLD4xnpi3YYX +kY0yZNkG+6wpn36NM2sfAHDgLYJRzSNgx+KxhTxDDbm3bHERawMGKo7H6abyen/YkEG 5p4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702520820; x=1703125620; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=BY8e/SFb9V+otPXNlhB6x6Lcjib+XLpc12tNaO8DAF0=; b=WEVAAKQxK/HrkiyRVgf0iQWh4jh8jH1sTKzornxExIUziI504AGQT+dizgSs3vmOGh Z/aIegxpZLLMS4sQgVUh/je18KhGNFJzrTb/+mGP+zzo5Fl7U7ylSFLbJVW7ITVi1A3S EcLJo4Qw9TSyf9i0Rjayt3797H0MuBIGcuTnChc7S0LkrSvBCt6IBsVhgt9eAjd/NRy3 EskuL9lWNqnt0OaVV8qA9Fv+mF4EaaBP/VVZxnZVNTKOqS6ieyklayZHsfgPbHZTH8uc efsGKNxN3GBhVZEX9f0oBX4hmOueu84DrsDt6xwyCgumZg/RcrzSKcp0lFjuBD9pxTjF V9ng== X-Gm-Message-State: AOJu0YybBTH5SWU70qi7sVGKBJmwnW4MSkw8AE60I9DIHQ3HTaQHzBSM cCWxEe3m2JwnEA/dg4pmldCDJMDb24+PC5aGvD4= X-Google-Smtp-Source: AGHT+IGHHIsnPQqjhXQ00nnF8wggHPKBNluUlTFo1aEq1VBHfAT0cBgC3Kp8krR0AYST0kQbsonBLQ1GFS4lvJk8gKk= X-Received: by 2002:a17:907:9058:b0:9e7:3af8:1fcd with SMTP id az24-20020a170907905800b009e73af81fcdmr3683212ejc.76.1702520819802; Wed, 13 Dec 2023 18:26:59 -0800 (PST) MIME-Version: 1.0 References: <20231211035243.15774-1-liangchen.linux@gmail.com> <20231211035243.15774-5-liangchen.linux@gmail.com> <20231211121409.5cfaebd5@kernel.org> In-Reply-To: From: Liang Chen Date: Thu, 14 Dec 2023 10:26:47 +0800 Message-ID: Subject: Re: [PATCH net-next v8 4/4] skbuff: Optimization of SKB coalescing for page pool To: Ilias Apalodimas Cc: Jakub Kicinski , davem@davemloft.net, edumazet@google.com, pabeni@redhat.com, hawk@kernel.org, linyunsheng@huawei.com, netdev@vger.kernel.org, linux-mm@kvack.org, jasowang@redhat.com, almasrymina@google.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: F1F33140018 X-Rspam-User: X-Stat-Signature: uftwtu7aj9qsycfm5bddzrspsjwzn34o X-Rspamd-Server: rspam01 X-HE-Tag: 1702520821-343116 X-HE-Meta: U2FsdGVkX19GLISoM5+UcRaOsM03q1kvaD3rOZHuXR7g6pnBI9pBDsHcQ9xECGin3wN/bMHqmvRIaa38rV3DzgbU8U0D4u3XQPLeo1vsEetHOUy/NJt6YYRaiqOub6QGl3m+WncECqIqNCd07eOcL3prnSfYW4cqTLykNcIgp7nX7/U3WkJVFHuLIuXQY16Pfbe07fc/FUv08EajvTaNhNtZfnle7OaXdvCf2vMjXITkiWue4PkrwPZzC7aXA1BEwe0Gb+JCeH1q0yJJkP9Va6eEmjQiLH3pMf3sUwlN4UWr0jZIKK9y+Un74iS3AlLTFFIZG9ab3Rnu5XqKBOzkoopjDLZWHIprfeCCU8ROpc2jWDFvyM3X4hhDk0m4b6wwB9BXPKpPNTjAAHjgUwUNc91xxkS0rwzDRKWJkwlq1r7TQC/s37SPZbUsHRAE8Z22S7FUzCZpzlZ3ED0x/wa8GOc3Ql2/tR/VcLmqKf+uhT34AVGmlSdvFX3+p6T+mxvRmwP8BMv8xvqdwjDc+cNP4KqrtUo6grAA/uOu2T++0DfUI+TFnn/qXPpIVf3Une968Jxv527pDTttX5KEZvBrMvUbBrdrcr9t3zWeizDM3P+XZsItImD/MQ22Sgxyb9DwlUAe3RL3XtZr8KaO40FfAhhIUHkfD3DHZ+xQMOQrGlfJqpjYkWfHQWdkF/1kpTxkyARBvAWl6pg0nlG+ZEUH+JtCFlGT+56q+bTR2cUouEmHO3VJIbiRXqyPiF+yqb8u2WDBJ1ykReaAs4VlPXlEcRV7VuOIuovCw1UlBqlHqq+mKDqGViaUK4KUznI9cfrq7MySiSjK8x27yTp/BE4uskd31faNHXa/1KtjEH7lr9hD0McUD+6AqmzADkIiJ8vWY6qhrrNcjZ4UEakq97ca/Kslx7Mtbxtpr5pIzYij6LhS6WOw4Lo7tW+zuSLUcgNl9xhkaN8eA8p89fEU+W0 zcOdr/Wp b2LPVn/8CHlu4vsL/L3NCgdhLAtIXmgQZuthlGd5oy1nNJDB+6aJEjmBObjMUFT7Q10olo1WBeNpEY+nMCW2pogV+1Yje7xijtMEeMdQg+CIO2FtXW4iVUEO6IF3uDqHKXnZ5mePX1oIuobzJlzq9qPczQ0BFYqbSVMPMOYKsc2Xt871whBlY8T/Ap9n5qsSuk8lAQqki9ieW5k7Q1P9r9mSck5fcaqYdTmNQyKbexkAX4az2qUzV5WZNS0/uPA9CEKBkIAbeh4VUGz2vqYIRiFFDPxiA6mDM0f4x+nqHs+SkL0E4WNOKiLDVQoDMR8IKIjleNwCWmvhRVBA7FjE7cZ3HlfGRdhSDL4x2Hn91gaHKrSGID4t4QZV/V1v21dXqNce+DDqfBc9uOyZ435DLV65zqzAx7vQBQF3Qycs4eT4sOjJbwNW1971xNZuYkuGBfoy893Ey5qpkM8R2a0ViFUgVmXOmLDL9Atb6+yDtem1qy8LWvMMcU4etzQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000006, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Dec 13, 2023 at 3:10=E2=80=AFPM Ilias Apalodimas wrote: > > Hi Jakub, > > On Mon, 11 Dec 2023 at 22:14, Jakub Kicinski wrote: > > > > On Mon, 11 Dec 2023 09:46:55 +0200 Ilias Apalodimas wrote: > > > As I said in the past the patch look correct. I don't like the fact > > > that more pp internals creep into the default network stack, but > > > perhaps this is fine with the bigger adoption? > > > Jakub any thoughts/objections? > > > > Now that you asked... the helper does seem to be in sort of > > a in-between state of being skb specific. > > > > What worries me is that this: > > > > +/** > > + * skb_pp_frag_ref() - Increase fragment reference count of a page > > + * @page: page of the fragment on which to increase a reference > > + * > > + * Increase fragment reference count (pp_ref_count) on a page, but if = it is > > + * not a page pool page, fallback to increase a reference(_refcount) o= n a > > + * normal page. > > + */ > > +static void skb_pp_frag_ref(struct page *page) > > +{ > > + struct page *head_page =3D compound_head(page); > > + > > + if (likely(is_pp_page(head_page))) > > + page_pool_ref_page(head_page); > > + else > > + page_ref_inc(head_page); > > +} > > > > doesn't even document that the caller must make sure that the skb > > which owns this page is marked for pp recycling. The caller added > > by this patch does that, but we should indicate somewhere that doing > > skb_pp_frag_ref() for frag in a non-pp-recycling skb is not correct. > > Correct > > > > > We can either lean in the direction of making it less skb specific, > > put the code in page_pool.c / helpers.h and make it clear that the > > caller has to be careful. > > Or we make it more skb specific, take a skb pointer as arg, and also > > look at its recycling marking.. > > or just improve the kdoc. > > I've mentioned this in the past, but I generally try to prevent people > from shooting themselves in the foot when creating APIs. Unless > there's a proven performance hit, I'd move the pp_recycle checking in > skb_pp_frag_ref(). > /** * skb_pp_frag_ref() - Increase fragment references of a page pool aware sk= b * @skb: page pool aware skb * * Increase the fragment reference count (pp_ref_count) of a skb. This is * intended to gain fragment references only for page pool aware skbs, * i.e. when skb->pp_recycle is true, and not for fragments in a * non-pp-recycling skb. It has a fallback to increase references on normal * pages, as page pool aware skbs may also have normal page fragments. */ Sure. Below is a snippet of the implementation for skb_pp_frag_ref, which takes an skb as its argument. The loop that iterates through the frags has been moved inside the function to avoid checking skb->pp_recycle each time a frag reference is taken(though there would be some optimization from the compiler). If there is no objection, it will be included in v10. Thanks! static int skb_pp_frag_ref(struct sk_buff *skb) { struct skb_shared_info *shinfo; struct page *head_page; int i; if (!skb->pp_recycle) return -EINVAL; shinfo =3D skb_shinfo(skb); for (i =3D 0; i < shinfo->nr_frags; i++){ head_page =3D compound_head(skb_frag_page(&shinfo->frags[i])); if (likely(is_pp_page(head_page))) page_pool_ref_page(head_page); else page_ref_inc(head_page); } return 0; } /* if the skb is not cloned this does nothing * since we set nr_frags to 0. */ if (skb_pp_frag_ref(from)) { for (i =3D 0; i < from_shinfo->nr_frags; i++) __skb_frag_ref(&from_shinfo->frags[i]); } > Thanks > /Ilias > > /Ilias