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 DEDB4C4332F for ; Wed, 13 Dec 2023 07:10:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6CBD96B00F1; Wed, 13 Dec 2023 02:10:14 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6525B6B00FF; Wed, 13 Dec 2023 02:10:14 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4CB506B0105; Wed, 13 Dec 2023 02:10:14 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 35BED6B00F1 for ; Wed, 13 Dec 2023 02:10:14 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 06FDB808DB for ; Wed, 13 Dec 2023 07:10:14 +0000 (UTC) X-FDA: 81560921148.21.8A3D777 Received: from mail-lj1-f181.google.com (mail-lj1-f181.google.com [209.85.208.181]) by imf10.hostedemail.com (Postfix) with ESMTP id 327C9C0002 for ; Wed, 13 Dec 2023 07:10:11 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=linaro.org header.s=google header.b=pyOYdVpG; dmarc=pass (policy=none) header.from=linaro.org; spf=pass (imf10.hostedemail.com: domain of ilias.apalodimas@linaro.org designates 209.85.208.181 as permitted sender) smtp.mailfrom=ilias.apalodimas@linaro.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1702451412; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=xr2xoEhq40SCymfewxeZeSzLkdKg7a4JO5PolVrdEF0=; b=qfXMvDd/0y7mofTaKBvhSXKwwHsQm2JcqQ5Qxm/6Pmg9WYTudpmzxaDzfgcBydt3IGZpQX Faeg24DCFi1UR4ZUZizcM8FhP73YmFVztSAFzMsLXJQCVRtUMy6R4TYs3+9h4QP03P8DMy O3oC76YOfzsqYmOO0+TaPgMcqhHDfW0= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=linaro.org header.s=google header.b=pyOYdVpG; dmarc=pass (policy=none) header.from=linaro.org; spf=pass (imf10.hostedemail.com: domain of ilias.apalodimas@linaro.org designates 209.85.208.181 as permitted sender) smtp.mailfrom=ilias.apalodimas@linaro.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1702451412; a=rsa-sha256; cv=none; b=pgZzCIZJbp8E8ATmgfJ1MDimmcdbVEuDqVdyLniLZLz9rv0jiibsN6BCWWs8Pj7Wk0KXKQ 1r0VEMv0QsXzaQi0bCJ11OsndOkcS8vbG7hR2Yb75HtgmTBYBPAm+5x3L7MonXSxRj/woq MNYWFwo2Zvm/V7Xs24T6EGCrRizcPVI= Received: by mail-lj1-f181.google.com with SMTP id 38308e7fff4ca-2cc2683fdaaso24755221fa.0 for ; Tue, 12 Dec 2023 23:10:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1702451410; x=1703056210; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=xr2xoEhq40SCymfewxeZeSzLkdKg7a4JO5PolVrdEF0=; b=pyOYdVpGOZE9EqPXntfoW1eMAHNuyNyEL8/ek1lPkJfygLOSdSIcWeDYGBuD8+y2Hl dIgCTB8ciE5IzsmawyKehiwXCZtvY4H5DnpCV8bDHshEVjV/ZX3+zUVV0lUjXCjicxPS vbBlh9lVW6I3Dw19qRtqgIPIlhmjP0PPLiYvuoSPQxuKTyRVQ8NX3RbQg06MtFwEoXxH OMg4TC5awAtWcTI2ROICENDTmxAECKqHKTbGsUfaQwizdEZeL5NEHIBsLaVVRX3fSI6S MLqNzTvjeT7Jt92zKTBRVTx88KdrwSTR+oj13slJfRVO4WkyxPcF16QHDcOFzPGJPHGR ypwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702451410; x=1703056210; h=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=xr2xoEhq40SCymfewxeZeSzLkdKg7a4JO5PolVrdEF0=; b=qKUiAGpI0k/UXYhA01mNPZSUZjnsD6dbyDlOzqSk3iclFDtbOE8FhB1dK89uYCIO9l mgkjStuRRzmT8RYhwWn9Y2RKewf2AZd7W97JBXpo97qCF2J34QPf9AUKMkJIZ5evtpmE euThYXe1d6ZM87pzj++Bx1lBjHOo9T2DgQCtt4oTDukWvRm988x526YxenGdJE+wRFNl GlrYuhflUMTIDtVkmAtzvVD9P0tl4agGzTZr72+sHorVuDG3HAcxn8xBt5tdEZwLx74T 0T4PdLQA/AYs85qzZEgMXhce706H27R5mMgIwmLgKxkMfCzeVZtCGbg2/MWsvM23hzBy xyjg== X-Gm-Message-State: AOJu0YzTDuP+QUpPeWYqHEeDBsbNX7qKYIfb6/R7rKUmhxB3tlgjKJxd eNeFUpd1QQ+1rwBNHKQBJns6H4ZPZ1786NMr8AT29A== X-Google-Smtp-Source: AGHT+IExKJAEseKKqyTNTppg1spHDP0V6SplJkXJNn8LRFc//Ja8pVqfqV5LsZJgybVitX2OtUydKq8VaOcBA3ALvoo= X-Received: by 2002:a2e:a99a:0:b0:2cb:2e9b:df9 with SMTP id x26-20020a2ea99a000000b002cb2e9b0df9mr3948145ljq.14.1702451410305; Tue, 12 Dec 2023 23:10:10 -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: <20231211121409.5cfaebd5@kernel.org> From: Ilias Apalodimas Date: Wed, 13 Dec 2023 09:09:34 +0200 Message-ID: Subject: Re: [PATCH net-next v8 4/4] skbuff: Optimization of SKB coalescing for page pool To: Jakub Kicinski Cc: Liang Chen , 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" X-Rspamd-Queue-Id: 327C9C0002 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: b4tbtozpgqciwbdx66kt1u7pqce9feqg X-HE-Tag: 1702451411-512255 X-HE-Meta: U2FsdGVkX1/Og2NzQECRb/+YEKrwThdabVFpG9UsnFvRosJ8H1tIzzFML6Mx2w/z/secd30Of8TCoy60S4phpUdcrvERixd2hEYA+Qk5HJpsBRn+Xj7paqFALe9rpbmTiS8OQ2XxmXY3ZBxnJ0SD7KCqhC2CBRlnPuweOdDSf3lx8qakM8/XSqsozvIlSSnIha7yx0KmGHLK0AMKEUch9JT85pFtwZG+CnAZFIpH2H1tYzgFvQoZ1C/pkKA3wuuq2mC7g7Ji023fZUptkKg3Xs60FdvvvvLzgI+1U01bnXgDYvs9IzKvGWwyy16Ez6HTVGbp01Qx2ReUtwRxML2jcuirZTvNj/YcTfzZ5qkL+UdtY5bw+3Ol5i6ZvRgAmPwhS/BkdHKp+3tvFMLNO6z5QdPHb31++In0SxgO18brglCDIGo8sU9FuctXTBOeNS7NYisTxANA0CLOqeMuPZKvRbX9GrFHtsYgFXgTQaYzzwwiTZ/Pm20EukXfC3c5ohy4ppw3HP1B5UX5cqATlDx35EiBTNIBNimfDHnlz8b80GFUCpdaR5SO5mVisuubtVklFOlbgxpayfAxkP43zIdao1hbmBSnv5i3BrUYkASS/KKhnZzZG8jfArIgUHNhwQyjzi+tj6+g0c269dEXDEz21MjRLAPFDgi3ZiHbrOiJtwDuFwxZgeZ9TYWAl89wVEchC55NOnhoaC8E6/8zAAjgdSXEz3y5ySeopmF+9ngg3Bzb/Yrn9KudaX5THUNC23Iw8ylJdM153txRptCwrAgDuJYkPvKqh+1j+IlGzitcYiMYPa6vcdQJBQEXVPbNb0T6Naoffl3j8WYTj4ttxhM9QJvYEOMzMpcpd1OYIDANwRn3an/5BiXem05MQ5l4B+iZc2FCVyY4pQAeuqjO36AbDcHIgwNEoYUenjM6ijJi/zstr+/xnv8v26W97ydwOYtNRnslNiuLRb+Wzr8eQVD 6/eTqFJr kM3i4LjyWzAp6dLXvTnb0ODe9YIXCsBgDxv+IXGEp2uSzftUA2zG/HWhxNTaS0zDCr1iJnS9EtnlSqpmz+LmgTaSHgEvHnWhPBuGoXdcFNp7lRd1eDzYdFr1/OrJSelYehgvHpBpkEIImqH9DyucfUvP/F3huBku4Z79IsZ70FTTNPdGsyJIPylunR5Ee8UAjKykm03xGZr+YC/r/y+ve97nev11XGO196DsJpZK+nw3HN91Ou4kYGdt/V5p8CxFPAYy7BAshfykk/2Z0X37WPiuquNxn9uaL7Gddy28EWLyBMaVjlWxeza2HdvycDUiclExecTV0r6oWC82iVjm0DnRyAGwfamZ04tBx X-Bogosity: Ham, tests=bogofilter, spamicity=0.000255, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: 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) on a > + * normal page. > + */ > +static void skb_pp_frag_ref(struct page *page) > +{ > + struct page *head_page = 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(). Thanks /Ilias /Ilias