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=-3.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no 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 1E680C433DB for ; Tue, 9 Feb 2021 17:49:35 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A8D2764E37 for ; Tue, 9 Feb 2021 17:49:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A8D2764E37 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linuxfoundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 34CF56B0005; Tue, 9 Feb 2021 12:49:34 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2FDCB6B006C; Tue, 9 Feb 2021 12:49:34 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 211F26B006E; Tue, 9 Feb 2021 12:49:34 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0085.hostedemail.com [216.40.44.85]) by kanga.kvack.org (Postfix) with ESMTP id 0A9386B0005 for ; Tue, 9 Feb 2021 12:49:34 -0500 (EST) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id C6E23180458DE for ; Tue, 9 Feb 2021 17:49:33 +0000 (UTC) X-FDA: 77799466626.08.fifth73_2017eed27609 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin08.hostedemail.com (Postfix) with ESMTP id A732818012C07 for ; Tue, 9 Feb 2021 17:49:33 +0000 (UTC) X-HE-Tag: fifth73_2017eed27609 X-Filterd-Recvd-Size: 6042 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf33.hostedemail.com (Postfix) with ESMTP for ; Tue, 9 Feb 2021 17:49:32 +0000 (UTC) Received: by mail.kernel.org (Postfix) with ESMTPSA id CA43D64E2F; Tue, 9 Feb 2021 17:49:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1612892971; bh=kvqH3pPX5LDjExP8UWGU7+aGNBkGtGwA6c29Yki1G/Q=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=kfQCsMZVeNCO1n8A99eeKPGg4wsmfZSq3yujcquPUvwyZ8TL5Zqd+oBq54VgkrrJ4 K88Lu8IZOJiXEh2jJzYGNvEVeKxcIK5iK5dlAVv9BOI6bBa88Z11/I+RA3+PgeoDjv T2oV2b4+OXfHNONKDwDk1oe84ploilQdSVU7xyog= Date: Tue, 9 Feb 2021 18:49:28 +0100 From: Greg KH To: Minchan Kim Cc: John Hubbard , Andrew Morton , linux-mm , LKML , surenb@google.com, joaodias@google.com, willy@infradead.org Subject: Re: [PATCH v2] mm: cma: support sysfs Message-ID: References: <20210208180142.2765456-1-minchan@kernel.org> <43cd6fc4-5bc5-50ec-0252-ffe09afd68ea@nvidia.com> <7cc229f4-609c-71dd-9361-063ef1bf7c73@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Feb 09, 2021 at 07:55:59AM -0800, Minchan Kim wrote: > On Tue, Feb 09, 2021 at 07:56:30AM +0100, Greg KH wrote: > > On Mon, Feb 08, 2021 at 10:34:51PM -0800, John Hubbard wrote: > > > On 2/8/21 10:27 PM, John Hubbard wrote: > > > > On 2/8/21 10:13 PM, Greg KH wrote: > > > > > On Mon, Feb 08, 2021 at 05:57:17PM -0800, John Hubbard wrote: > > > > > > On 2/8/21 3:36 PM, Minchan Kim wrote: > > > > > > ... > > > > > > > > > =A0=A0=A0=A0=A0=A0=A0 char name[CMA_MAX_NAME]; > > > > > > > > > +#ifdef CONFIG_CMA_SYSFS > > > > > > > > > +=A0=A0=A0 struct cma_stat=A0=A0=A0 *stat; > > > > > > > >=20 > > > > > > > > This should not be a pointer. By making it a pointer, you= 've added a bunch of pointless > > > > > > > > extra code to the implementation. > > > > > > >=20 > > > > > > > Originally, I went with the object lifetime with struct cma= as you > > > > > > > suggested to make code simple. However, Greg KH wanted to h= ave > > > > > > > release for kobj_type since it is consistent with other kbo= ject > > > > > > > handling. > > > > > >=20 > > > > > > Are you talking about the kobj in your new struct cma_stat? T= hat seems > > > > > > like circular logic if so. I'm guessing Greg just wanted kobj= methods > > > > > > to be used *if* you are dealing with kobjects. That's a narro= wer point. > > > > > >=20 > > > > > > I can't imagine that he would have insisted on having additio= nal > > > > > > allocations just so that kobj freeing methods could be used. = :) > > > > >=20 > > > > > Um, yes, I was :) > > > > >=20 > > > > > You can not add a kobject to a structure and then somehow think= you can > > > > > just ignore the reference counting issues involved.=A0 If a kob= ject is > > > > > part of a structure then the kobject is responsible for control= ing the > > > > > lifespan of the memory, nothing else can be. > > > > >=20 > > > > > So by making the kobject dynamic, you properly handle that memo= ry > > > > > lifespan of the object, instead of having to worry about the li= fespan of > > > > > the larger object (which the original patch was not doing.) > > > > >=20 > > > > > Does that make sense? > > > > >=20 > > > > That part makes sense, yes, thanks. The part that I'm trying to s= traighten > > > > out is, why was kobject even added to the struct cma_stat in the = first > > > > place? Why not just leave .stat as a static member variable, with= out > > > > a kobject in it, and done? > > > >=20 > > >=20 > > > Sorry, I think I get it now: this is in order to allow a separate l= ifetime > > > for the .stat member. I was sort of implicitly assuming that the "r= ight" > > > way to do it is just have the whole object use one lifetime managem= ent, > > > but as you say, there is no kobject being added to the parent. > > >=20 > > > I still feel odd about the allocation and freeing of something that= seems > > > to be logically the same lifetime (other than perhaps a few, briefl= y pending > > > sysfs reads, at the end of life). So I'd still think that the kobje= ct should > > > be added to the parent... >=20 > sruct cma_stat { > spinlock_t lock; > unsigned long pages_attemtp; > unsigned long pages_fail; > }; >=20 > struct cma { > .. > .. > struct kobject kobj; > struct cma_stat stat; > }; >=20 > I guess this is what Johan suggested. I agree with it. >=20 > >=20 > > That's fine if you want to add it to the parent. If so, then the > > kobject controls the lifetime of the structure, nothing else can. >=20 > The problem was parent object(i.e., struct cma cma_areas) is > static arrary so kobj->release function will be NULL or just > dummy. Is it okay? I thought it was one of the what you wanted > to avoid it. No, that is not ok. > > Either is fine with me, what is "forbidden" is having a kobject and > > somehow thinking that it does not control the lifetime of the structu= re. >=20 > Since parent object is static arrary, there is no need to control the > lifetime so I am curious if parent object approach is okay from kobject > handling point of view. So the array is _NEVER_ freed? If not, fine, don't provide a release function for the kobject, but ick, just make a dynamic kobject I don't see the problem for something so tiny and not very many... I worry that any static kobject might be copied/pasted as someone might think this is an ok thing to do. And it's not an ok thing to do. thanks, greg k-h