Re: [RFC PATCH 10/13] mm: Introduce first class virtual address spaces

[Till Smejkal <till.smejkal@googlemail.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