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
