From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Hubbard Subject: Re: [PATCH 3/3] net/xdp: convert put_page() to put_user_page*() Date: Mon, 22 Jul 2019 21:41:34 -0700 Message-ID: References: <20190722223415.13269-1-jhubbard@nvidia.com> <20190722223415.13269-4-jhubbard@nvidia.com> <20190723002534.GA10284@iweiny-DESK2.sc.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20190723002534.GA10284@iweiny-DESK2.sc.intel.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Ira Weiny , john.hubbard@gmail.com Cc: Andrew Morton , Alexander Viro , =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= , Boaz Harrosh , Christoph Hellwig , Daniel Vetter , Dan Williams , Dave Chinner , David Airlie , "David S . Miller" , Ilya Dryomov , Jan Kara , Jason Gunthorpe , Jens Axboe , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , Johannes Thumshirn , Magnus Karlsson , Matthew Wilcox , Miklos Szeredi , Ming Lei List-Id: dri-devel@lists.freedesktop.org On 7/22/19 5:25 PM, Ira Weiny wrote: > On Mon, Jul 22, 2019 at 03:34:15PM -0700, john.hubbard@gmail.com wrote: >> From: John Hubbard >> >> For pages that were retained via get_user_pages*(), release those pages >> via the new put_user_page*() routines, instead of via put_page() or >> release_pages(). >> >> This is part a tree-wide conversion, as described in commit fc1d8e7cca2d >> ("mm: introduce put_user_page*(), placeholder versions"). >> >> Cc: Bj=C3=B6rn T=C3=B6pel >> Cc: Magnus Karlsson >> Cc: David S. Miller >> Cc: netdev@vger.kernel.org >> Signed-off-by: John Hubbard >> --- >> net/xdp/xdp_umem.c | 9 +-------- >> 1 file changed, 1 insertion(+), 8 deletions(-) >> >> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c >> index 83de74ca729a..0325a17915de 100644 >> --- a/net/xdp/xdp_umem.c >> +++ b/net/xdp/xdp_umem.c >> @@ -166,14 +166,7 @@ void xdp_umem_clear_dev(struct xdp_umem *umem) >> =20 >> static void xdp_umem_unpin_pages(struct xdp_umem *umem) >> { >> - unsigned int i; >> - >> - for (i =3D 0; i < umem->npgs; i++) { >> - struct page *page =3D umem->pgs[i]; >> - >> - set_page_dirty_lock(page); >> - put_page(page); >> - } >> + put_user_pages_dirty_lock(umem->pgs, umem->npgs); >=20 > What is the difference between this and >=20 > __put_user_pages(umem->pgs, umem->npgs, PUP_FLAGS_DIRTY_LOCK); >=20 > ? No difference. >=20 > I'm a bit concerned with adding another form of the same interface. We s= hould > either have 1 call with flags (enum in this case) or multiple calls. Giv= en the > previous discussion lets move in the direction of having the enum but don= 't > introduce another caller of the "old" interface. I disagree that this is a "problem". There is no maintenance pitfall here; = there are merely two ways to call the put_user_page*() API. Both are correct, and neither one will get you into trouble. Not only that, but there is ample precedent for this approach in other kernel APIs. >=20 > So I think on this patch NAK from me. >=20 > I also don't like having a __* call in the exported interface but there i= s a > __get_user_pages_fast() call so I guess there is precedent. :-/ >=20 I thought about this carefully, and looked at other APIs. And I noticed tha= t things like __get_user_pages*() are how it's often done: * The leading underscores are often used for the more elaborate form of the call (as oppposed to decorating the core function name with "_flags", for example). * There are often calls in which you can either call the simpler form, or t= he form with flags and additional options, and yes, you'll get the same result= . Obviously, this stuff is all subject to a certain amount of opinion, but I think I'm on really solid ground as far as precedent goes. So I'm pushing back on the NAK... :) thanks, --=20 John Hubbard NVIDIA