From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-f71.google.com (mail-pg0-f71.google.com [74.125.83.71]) by kanga.kvack.org (Postfix) with ESMTP id F304C6B038A for ; Mon, 13 Mar 2017 19:52:40 -0400 (EDT) Received: by mail-pg0-f71.google.com with SMTP id f21so333815877pgi.4 for ; Mon, 13 Mar 2017 16:52:40 -0700 (PDT) Date: Tue, 14 Mar 2017 07:52:25 +0800 From: Greg Kroah-Hartman Subject: Re: [RFC PATCH 10/13] mm: Introduce first class virtual address spaces Message-ID: <20170313235225.GA15359@kroah.com> References: <20170313221415.9375-1-till.smejkal@gmail.com> <20170313221415.9375-11-till.smejkal@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170313221415.9375-11-till.smejkal@gmail.com> Sender: owner-linux-mm@kvack.org List-ID: To: Till Smejkal Cc: Richard Henderson , Ivan Kokshaysky , Matt Turner , Vineet Gupta , Russell King , Catalin Marinas , Will Deacon , Steven Miao , Richard Kuo , Tony Luck , Fenghua Yu , James Hogan , Ralf Baechle , "James E.J. Bottomley" , Helge Deller , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , Martin Schwidefsky , Heiko Carstens , Yoshinori Sato , Rich Felker , "David S. Miller" , Chris Metcalf , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Andy Lutomirski , Chris Zankel , Max Filippov , Arnd Bergmann , Laurent Pinchart , Mauro Carvalho Chehab , Pawel Osciak , Marek Szyprowski , Kyungmin Park , David Woodhouse , Brian Norris , Boris Brezillon , Marek Vasut , Richard Weinberger , Cyrille Pitchen , Felipe Balbi , Alexander Viro , Benjamin LaHaise , Nadia Yvette Chambers , Jeff Layton , "J. Bruce Fields" , Peter Zijlstra , Hugh Dickins , Arnaldo Carvalho de Melo , Alexander Shishkin , Jaroslav Kysela , Takashi Iwai , linux-kernel@vger.kernel.org, linux-alpha@vger.kernel.org, linux-snps-arc@lists.infradead.org, linux-arm-kernel@lists.infradead.org, adi-buildroot-devel@lists.sourceforge.net, linux-hexagon@vger.kernel.org, linux-ia64@vger.kernel.org, linux-metag@vger.kernel.org, linux-mips@linux-mips.org, linux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, sparclinux@vger.kernel.org, linux-xtensa@linux-xtensa.org, linux-media@vger.kernel.org, linux-mtd@lists.infradead.org, linux-usb@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-aio@kvack.org, linux-mm@kvack.org, linux-api@vger.kernel.org, linux-arch@vger.kernel.org, alsa-devel@alsa-project.org On Mon, Mar 13, 2017 at 03:14:12PM -0700, Till Smejkal wrote: There's no way with that many cc: lists and people that this is really making it through very many people's filters and actually on a mailing list. Please trim them down. Minor sysfs questions/issues: > +struct vas { > + struct kobject kobj; /* < the internal kobject that we use * > + * for reference counting and sysfs * > + * handling. */ > + > + int id; /* < ID */ > + char name[VAS_MAX_NAME_LENGTH]; /* < name */ The kobject has a name, why not use that? > + > + struct mutex mtx; /* < lock for parallel access. */ > + > + struct mm_struct *mm; /* < a partial memory map containing * > + * all mappings of this VAS. */ > + > + struct list_head link; /* < the link in the global VAS list. */ > + struct rcu_head rcu; /* < the RCU helper used for * > + * asynchronous VAS deletion. */ > + > + u16 refcount; /* < how often is the VAS attached. */ The kobject has a refcount, use that? Don't have 2 refcounts in the same structure, that way lies madness. And bugs, lots of bugs... And if this really is a refcount (hint, I don't think it is), you should use the refcount_t type. > +/** > + * The sysfs structure we need to handle attributes of a VAS. > + **/ > +struct vas_sysfs_attr { > + struct attribute attr; > + ssize_t (*show)(struct vas *vas, struct vas_sysfs_attr *vsattr, > + char *buf); > + ssize_t (*store)(struct vas *vas, struct vas_sysfs_attr *vsattr, > + const char *buf, size_t count); > +}; > + > +#define VAS_SYSFS_ATTR(NAME, MODE, SHOW, STORE) \ > +static struct vas_sysfs_attr vas_sysfs_attr_##NAME = \ > + __ATTR(NAME, MODE, SHOW, STORE) __ATTR_RO and __ATTR_RW should work better for you. If you really need this. Oh, and where is the Documentation/ABI/ updates to try to describe the sysfs structure and files? Did I miss that in the series? > +static ssize_t __show_vas_name(struct vas *vas, struct vas_sysfs_attr *vsattr, > + char *buf) > +{ > + return scnprintf(buf, PAGE_SIZE, "%s", vas->name); It's a page size, just use sprintf() and be done with it. No need to ever check, you "know" it will be correct. Also, what about a trailing '\n' for these attributes? Oh wait, why have a name when the kobject name is already there in the directory itself? Do you really need this? > +/** > + * The ktype data structure representing a VAS. > + **/ > +static struct kobj_type vas_ktype = { > + .sysfs_ops = &vas_sysfs_ops, > + .release = __vas_release, Why the odd __vas* naming? What's wrong with vas_release? > + .default_attrs = vas_default_attr, > +}; > + > + > +/*** > + * Internally visible functions > + ***/ > + > +/** > + * Working with the global VAS list. > + **/ > +static inline void vas_remove(struct vas *vas) You have a ton of inline functions, for no good reason. Make them all "real" functions please. Unless you can measure the size/speed differences? If so, please say so. thanks, greg k-h -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org