Re: [RFC PATCH 10/13] mm: Introduce first class virtual address spaces
Hi Vineet, On Mon, 13 Mar 2017, Vineet Gupta wrote: > I've not looked at the patches closely (or read the references paper fully > yet), > but at first glance it seems on ARC architecture, we can can potentially > use/leverage this mechanism to implement the shared TLB entries. Before anyone > shouts these are not same as the IA64/x86 protection keys which allow TLB > entries > with different protection bits across processes etc. These TLB entries are > actually *shared* by processes. > > Conceptually there's shared address spaces, independent of processes. e.g. > ldso > code is shared address space #1, libc (code) #2 System can support a > limited > number of shared addr spaces (say 64, enough for typical embedded sys). > > While Normal TLB entries are tagged with ASID (Addr space ID) to keep them > unique > across processes, Shared TLB entries are tagged with Shared address space ID. > > A process MMU context consists of ASID (a single number) and a SASID bitmap > (to > allow "subscription" to multiple Shared spaces. The subscriptions are set up > bu > userspace ld.so which knows about the libs process wants to map. > > The restriction ofcourse is that the spaces are mapped at *same* vaddr is all > participating processes. I know this goes against whole security, address > space > randomization - but it gives much better real time performance. Why does each > process need to take a MMU exception for libc code... > > So long story short - it seems there can be multiple uses of this > infrastructure ! During the development of this code, we also looked at shared TLB entries, but the other way around. We wanted to use them to prevent flushing of TLB entries of shared memory regions when switching between multiple ASes. Unfortunately, we never finished this part of the code. However, we also investigated into a different use-case for first class virtual address spaces that is related to what you propose if I didn't understand something wrong. The idea is to move shared libraries into their own first class virtual address space and only load some small trampoline code in the application AS. This trampoline code performs the VAS switch in the libraries AS and execute the requested function there. If we combine this architecture with tagged TLB entries to prevent TLB flushes during the switch operation, it can also reach an acceptable performance. A side effect of moving the shared library into its own AS is that it can not be used by ROP-attacks because it is not accessible in the application's AS. Till ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RFC PATCH 10/13] mm: Introduce first class virtual address spaces
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 = _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)
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
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [RFC PATCH 10/13] mm: Introduce first class virtual address spaces
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 = _sysfs_ops,
> > + .release = __vas_release,
>
> Why the odd
