From: Till Smejkal <till.smejkal@googlemail.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Till Smejkal <till.smejkal@googlemail.com>,
Richard Henderson <rth@twiddle.net>,
Ivan Kokshaysky <ink@jurassic.park.msu.ru>,
Matt Turner <mattst88@gmail.com>,
Vineet Gupta <vgupta@synopsys.com>,
Russell King <linux@armlinux.org.uk>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will.deacon@arm.com>,
Steven Miao <realmz6@gmail.com>,
Richard Kuo <rkuo@codeaurora.org>,
Tony Luck <tony.luck@intel.com>,
Fenghua Yu <fenghua.yu@intel.com>,
James Hogan <james.hogan@imgtec.com>,
Ralf Baechle <ralf@linux-mips.org>,
"James E.J. Bottomley" <jejb@parisc-linux.org>,
Helge Deller <deller@gmx.de>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Michael Ellerman <mpe@ellerman.id.au>,
Martin Schwidefsky <schwidefsky@de.ibm.com>,
Heiko Carstens <heiko.carstens@de.ibm.com>,
Yoshinori Sato <ysato@users.sourceforge.jp>, Rich Felker <dalia>
Subject: Re: [RFC PATCH 10/13] mm: Introduce first class virtual address spaces
Date: Mon, 13 Mar 2017 17:24:48 -0700 [thread overview]
Message-ID: <20170314002448.hwbxlasbbz3shhlv@arch-dev> (raw)
In-Reply-To: <20170313235225.GA15359@kroah.com>
Hi Greg,
First of all thanks for your reply.
On Tue, 14 Mar 2017, Greg Kroah-Hartman wrote:
> 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.
I am sorry that the patch's cc-list is too big. This was the list of people that the
get_maintainers.pl script produced. I already recognized that it was a huge number of
people, but I didn't want to remove anyone from the list because I wasn't sure who
would be interested in this patch set. Do you have any suggestion who to remove from
the list? I don't want to annoy anyone with useless emails.
> 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?
The reason why I don't use the kobject's name is that I don't restrict the names that
are used for VAS/VAS segments. Accordingly, it would be allowed to use a name like
"foo/bar/xyz" as VAS name. However, I am not sure what would happen in the sysfs if I
would use such a name for the kobject. Especially, since one could think of another
VAS with the name "foo/bar" whose name would conflict with the first one although it
not necessarily has any connection with it.
> > +
> > + 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.
I actually use both the internal kobject refcount to keep track of how often a
VAS/VAS segment is referenced and this 'refcount' variable to keep track how often
the VAS is actually attached to a task. They not necessarily must be related to each
other. I can rename this variable to attach_count. Or if preferred I can
alternatively only use the kobject reference counter and remove this variable
completely though I would loose information about how often the VAS is attached to a
task because the kobject reference counter is also used to keep track of other
variables referencing the VAS.
> > +/**
> > + * 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.
Thank you. I will have a look at these functions.
> Oh, and where is the Documentation/ABI/ updates to try to describe the
> sysfs structure and files? Did I miss that in the series?
Oh sorry, I forgot to add this file. I will add the ABI descriptions for future
submissions.
> > +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.
OK. I was following the sysfs example in the documentation that used scnprintf, but
if sprintf is preferred, I can change this.
> Also, what about a trailing '\n' for these attributes?
I will change this.
> Oh wait, why have a name when the kobject name is already there in the
> directory itself? Do you really need this?
See above.
> > +/**
> > + * 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?
I was using the __* naming scheme for functions that have no other meaning outside of
my source file. But I can change this if people don't like it. I have no strong
feelings about the names of the functions.
> > + .default_attrs = vas_default_attr,
> > +};
> > +
> > +
> > +/***
> > + * Internally visible functions
> > + ***/
> > +
> > +/**
> > + * Working with the global VAS list.
> > + **/
> > +static inline void vas_remove(struct vas *vas)
>
> <snip>
>
> 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.
There was no specific reason why I declared the functions as inline except my hope to
reduce the function call for some of my very small functions. I can look more closely
at this and check whether there is some real benefit in inlining them and if not
remove it.
Thank you very much.
Till
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
next prev parent reply other threads:[~2017-03-14 0:24 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-13 22:14 [RFC PATCH 00/13] Introduce first class virtual address spaces Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 01/13] mm: Add mm_struct argument to 'mmap_region' Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 02/13] mm: Add mm_struct argument to 'do_mmap' and 'do_mmap_pgoff' Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 03/13] mm: Rename 'unmap_region' and add mm_struct argument Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 04/13] mm: Add mm_struct argument to 'get_unmapped_area' and 'vm_unmapped_area' Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 05/13] mm: Add mm_struct argument to 'mm_populate' and '__mm_populate' Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 06/13] mm/mmap: Export 'vma_link' and 'find_vma_links' to mm subsystem Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 07/13] kernel/fork: Split and export 'mm_alloc' and 'mm_init' Till Smejkal
2017-03-14 10:18 ` David Laight
2017-03-14 16:18 ` Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 08/13] kernel/fork: Define explicitly which mm_struct to duplicate during fork Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 09/13] mm/memory: Add function to one-to-one duplicate page ranges Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 10/13] mm: Introduce first class virtual address spaces Till Smejkal
2017-03-13 23:52 ` Greg Kroah-Hartman
2017-03-14 0:24 ` Till Smejkal [this message]
2017-03-14 1:35 ` Vineet Gupta
2017-03-14 2:34 ` Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 11/13] mm/vas: Introduce VAS segments - shareable address space regions Till Smejkal
2017-03-13 22:27 ` Matthew Wilcox
2017-03-13 22:45 ` Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 12/13] mm/vas: Add lazy-attach support for first class virtual address spaces Till Smejkal
2017-03-13 22:14 ` [RFC PATCH 13/13] fs/proc: Add procfs " Till Smejkal
2017-03-14 0:18 ` [RFC PATCH 00/13] Introduce " Richard Henderson
2017-03-14 0:39 ` Till Smejkal
2017-03-14 0:58 ` Andy Lutomirski
2017-03-14 2:07 ` Till Smejkal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170314002448.hwbxlasbbz3shhlv@arch-dev \
--to=till.smejkal@googlemail.com \
--cc=benh@kernel.crashing.org \
--cc=catalin.marinas@arm.com \
--cc=deller@gmx.de \
--cc=fenghua.yu@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=heiko.carstens@de.ibm.com \
--cc=ink@jurassic.park.msu.ru \
--cc=james.hogan@imgtec.com \
--cc=jejb@parisc-linux.org \
--cc=linux@armlinux.org.uk \
--cc=mattst88@gmail.com \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.org \
--cc=ralf@linux-mips.org \
--cc=realmz6@gmail.com \
--cc=rkuo@codeaurora.org \
--cc=rth@twiddle.net \
--cc=schwidefsky@de.ibm.com \
--cc=tony.luck@intel.com \
--cc=vgupta@synopsys.com \
--cc=will.deacon@arm.com \
--cc=ysato@users.sourceforge.jp \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).