Re: [RFC PATCH 10/13] mm: Introduce first class virtual address spaces
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: 2017-03-13 23:52:25
Also in:
alsa-devel, linux-alpha, linux-api, linux-arch, linux-fsdevel, linux-mips, linux-mm
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:
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ + 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.
quoted hunk ↗ jump to hunk
+/** + * 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?
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+/** + * 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?
quoted hunk ↗ jump to hunk
+ .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