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

[Greg Kroah-Hartman <gregkh@linuxfoundation.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)

<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.


thanks,

greg k-h

--
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>