| Message ID | 20201201082128.15239-13-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: support per-cpupool scheduling granularity | expand |
On 01.12.2020 09:21, Juergen Gross wrote: > In order to better support resource allocation and locking for dynamic > hypfs nodes add enter() and exit() callbacks to struct hypfs_funcs. > > The enter() callback is called when entering a node during hypfs user > actions (traversing, reading or writing it), while the exit() callback > is called when leaving a node (accessing another node at the same or a > higher directory level, or when returning to the user). > > For avoiding recursion this requires a parent pointer in each node. > Let the enter() callback return the entry address which is stored as > the last accessed node in order to be able to use a template entry for > that purpose in case of dynamic entries. I guess I'll learn in subsequent patches why this is necessary / useful. Right now it looks odd for the function to simple return the incoming argument, as this way it's clear the caller knows the correct value already. > @@ -100,11 +112,58 @@ static void hypfs_unlock(void) > } > } > > +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) > +{ > + return entry; > +} > + > +void hypfs_node_exit(const struct hypfs_entry *entry) > +{ > +} > + > +static int node_enter(const struct hypfs_entry *entry) > +{ > + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); > + > + entry = entry->funcs->enter(entry); > + if ( IS_ERR(entry) ) > + return PTR_ERR(entry); > + > + ASSERT(!*last || *last == entry->parent); > + > + *last = entry; > + > + return 0; > +} > + > +static void node_exit(const struct hypfs_entry *entry) > +{ > + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); > + > + if ( !*last ) > + return; Under what conditions is this legitimate to happen? IOW shouldn't there be an ASSERT_UNREACHABLE() here? Jan
On 03.12.20 15:59, Jan Beulich wrote: > On 01.12.2020 09:21, Juergen Gross wrote: >> In order to better support resource allocation and locking for dynamic >> hypfs nodes add enter() and exit() callbacks to struct hypfs_funcs. >> >> The enter() callback is called when entering a node during hypfs user >> actions (traversing, reading or writing it), while the exit() callback >> is called when leaving a node (accessing another node at the same or a >> higher directory level, or when returning to the user). >> >> For avoiding recursion this requires a parent pointer in each node. >> Let the enter() callback return the entry address which is stored as >> the last accessed node in order to be able to use a template entry for >> that purpose in case of dynamic entries. > > I guess I'll learn in subsequent patches why this is necessary / > useful. Right now it looks odd for the function to simple return > the incoming argument, as this way it's clear the caller knows > the correct value already. Basically for dynamic entries based on a template the entry function will return the address of template->e instead of the dynamic entry itself. This will enable to have the standard entry functions for any nodes being linked to the template. > >> @@ -100,11 +112,58 @@ static void hypfs_unlock(void) >> } >> } >> >> +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) >> +{ >> + return entry; >> +} >> + >> +void hypfs_node_exit(const struct hypfs_entry *entry) >> +{ >> +} >> + >> +static int node_enter(const struct hypfs_entry *entry) >> +{ >> + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); >> + >> + entry = entry->funcs->enter(entry); >> + if ( IS_ERR(entry) ) >> + return PTR_ERR(entry); >> + >> + ASSERT(!*last || *last == entry->parent); >> + >> + *last = entry; >> + >> + return 0; >> +} >> + >> +static void node_exit(const struct hypfs_entry *entry) >> +{ >> + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); >> + >> + if ( !*last ) >> + return; > > Under what conditions is this legitimate to happen? IOW shouldn't > there be an ASSERT_UNREACHABLE() here? This is for the "/" node. Juergen
On 03.12.2020 16:14, Jürgen Groß wrote: > On 03.12.20 15:59, Jan Beulich wrote: >> On 01.12.2020 09:21, Juergen Gross wrote: >>> @@ -100,11 +112,58 @@ static void hypfs_unlock(void) >>> } >>> } >>> >>> +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) >>> +{ >>> + return entry; >>> +} >>> + >>> +void hypfs_node_exit(const struct hypfs_entry *entry) >>> +{ >>> +} >>> + >>> +static int node_enter(const struct hypfs_entry *entry) >>> +{ >>> + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); >>> + >>> + entry = entry->funcs->enter(entry); >>> + if ( IS_ERR(entry) ) >>> + return PTR_ERR(entry); >>> + >>> + ASSERT(!*last || *last == entry->parent); >>> + >>> + *last = entry; >>> + >>> + return 0; >>> +} >>> + >>> +static void node_exit(const struct hypfs_entry *entry) >>> +{ >>> + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); >>> + >>> + if ( !*last ) >>> + return; >> >> Under what conditions is this legitimate to happen? IOW shouldn't >> there be an ASSERT_UNREACHABLE() here? > > This is for the "/" node. I.e. would ASSERT(!entry->parent) be appropriate to add here, at the same time serving as documentation of what you've just said? Jan
On 01.12.2020 09:21, Juergen Gross wrote: > @@ -100,11 +112,58 @@ static void hypfs_unlock(void) > } > } > > +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) > +{ > + return entry; > +} > + > +void hypfs_node_exit(const struct hypfs_entry *entry) > +{ > +} > + > +static int node_enter(const struct hypfs_entry *entry) > +{ > + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); > + > + entry = entry->funcs->enter(entry); > + if ( IS_ERR(entry) ) > + return PTR_ERR(entry); > + > + ASSERT(!*last || *last == entry->parent); > + > + *last = entry; In such a callback case I wonder whether you wouldn't want to also guard against NULL coming back, perhaps simply by mistake, but of course once handling it here such could even become permissible behavior. Jan
On 03.12.20 16:29, Jan Beulich wrote: > On 03.12.2020 16:14, Jürgen Groß wrote: >> On 03.12.20 15:59, Jan Beulich wrote: >>> On 01.12.2020 09:21, Juergen Gross wrote: >>>> @@ -100,11 +112,58 @@ static void hypfs_unlock(void) >>>> } >>>> } >>>> >>>> +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) >>>> +{ >>>> + return entry; >>>> +} >>>> + >>>> +void hypfs_node_exit(const struct hypfs_entry *entry) >>>> +{ >>>> +} >>>> + >>>> +static int node_enter(const struct hypfs_entry *entry) >>>> +{ >>>> + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); >>>> + >>>> + entry = entry->funcs->enter(entry); >>>> + if ( IS_ERR(entry) ) >>>> + return PTR_ERR(entry); >>>> + >>>> + ASSERT(!*last || *last == entry->parent); >>>> + >>>> + *last = entry; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void node_exit(const struct hypfs_entry *entry) >>>> +{ >>>> + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); >>>> + >>>> + if ( !*last ) >>>> + return; >>> >>> Under what conditions is this legitimate to happen? IOW shouldn't >>> there be an ASSERT_UNREACHABLE() here? >> >> This is for the "/" node. > > I.e. would ASSERT(!entry->parent) be appropriate to add here, at > the same time serving as documentation of what you've just said? I rechecked and have found that this was a remnant from an earlier variant. *last won't ever be NULL, so the if can be dropped (a NULL will be catched by the following ASSERT()). Juergen
On 04.12.20 09:30, Jan Beulich wrote: > On 01.12.2020 09:21, Juergen Gross wrote: >> @@ -100,11 +112,58 @@ static void hypfs_unlock(void) >> } >> } >> >> +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) >> +{ >> + return entry; >> +} >> + >> +void hypfs_node_exit(const struct hypfs_entry *entry) >> +{ >> +} >> + >> +static int node_enter(const struct hypfs_entry *entry) >> +{ >> + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); >> + >> + entry = entry->funcs->enter(entry); >> + if ( IS_ERR(entry) ) >> + return PTR_ERR(entry); >> + >> + ASSERT(!*last || *last == entry->parent); >> + >> + *last = entry; > > In such a callback case I wonder whether you wouldn't want to also > guard against NULL coming back, perhaps simply by mistake, but of > course once handling it here such could even become permissible > behavior. I think you are right. I should add an ASSERT(entry); Juergen
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c index 83c5cacdca..e5adc9defe 100644 --- a/xen/common/hypfs.c +++ b/xen/common/hypfs.c @@ -25,30 +25,40 @@ CHECK_hypfs_dirlistentry; ROUNDUP((name_len) + 1, alignof(struct xen_hypfs_direntry))) const struct hypfs_funcs hypfs_dir_funcs = { + .enter = hypfs_node_enter, + .exit = hypfs_node_exit, .read = hypfs_read_dir, .write = hypfs_write_deny, .getsize = hypfs_getsize, .findentry = hypfs_dir_findentry, }; const struct hypfs_funcs hypfs_leaf_ro_funcs = { + .enter = hypfs_node_enter, + .exit = hypfs_node_exit, .read = hypfs_read_leaf, .write = hypfs_write_deny, .getsize = hypfs_getsize, .findentry = hypfs_leaf_findentry, }; const struct hypfs_funcs hypfs_leaf_wr_funcs = { + .enter = hypfs_node_enter, + .exit = hypfs_node_exit, .read = hypfs_read_leaf, .write = hypfs_write_leaf, .getsize = hypfs_getsize, .findentry = hypfs_leaf_findentry, }; const struct hypfs_funcs hypfs_bool_wr_funcs = { + .enter = hypfs_node_enter, + .exit = hypfs_node_exit, .read = hypfs_read_leaf, .write = hypfs_write_bool, .getsize = hypfs_getsize, .findentry = hypfs_leaf_findentry, }; const struct hypfs_funcs hypfs_custom_wr_funcs = { + .enter = hypfs_node_enter, + .exit = hypfs_node_exit, .read = hypfs_read_leaf, .write = hypfs_write_custom, .getsize = hypfs_getsize, @@ -63,6 +73,8 @@ enum hypfs_lock_state { }; static DEFINE_PER_CPU(enum hypfs_lock_state, hypfs_locked); +static DEFINE_PER_CPU(const struct hypfs_entry *, hypfs_last_node_entered); + HYPFS_DIR_INIT(hypfs_root, ""); static void hypfs_read_lock(void) @@ -100,11 +112,58 @@ static void hypfs_unlock(void) } } +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) +{ + return entry; +} + +void hypfs_node_exit(const struct hypfs_entry *entry) +{ +} + +static int node_enter(const struct hypfs_entry *entry) +{ + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); + + entry = entry->funcs->enter(entry); + if ( IS_ERR(entry) ) + return PTR_ERR(entry); + + ASSERT(!*last || *last == entry->parent); + + *last = entry; + + return 0; +} + +static void node_exit(const struct hypfs_entry *entry) +{ + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); + + if ( !*last ) + return; + + ASSERT(*last == entry); + *last = entry->parent; + + entry->funcs->exit(entry); +} + +static void node_exit_all(void) +{ + const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); + + while ( *last ) + node_exit(*last); +} + static int add_entry(struct hypfs_entry_dir *parent, struct hypfs_entry *new) { int ret = -ENOENT; struct hypfs_entry *e; + ASSERT(new->funcs->enter); + ASSERT(new->funcs->exit); ASSERT(new->funcs->read); ASSERT(new->funcs->write); ASSERT(new->funcs->getsize); @@ -140,6 +199,7 @@ static int add_entry(struct hypfs_entry_dir *parent, struct hypfs_entry *new) unsigned int sz = strlen(new->name); parent->e.size += DIRENTRY_SIZE(sz); + new->parent = &parent->e; } hypfs_unlock(); @@ -221,6 +281,7 @@ static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir, const char *end; struct hypfs_entry *entry; unsigned int name_len; + int ret; for ( ; ; ) { @@ -235,6 +296,10 @@ static struct hypfs_entry *hypfs_get_entry_rel(struct hypfs_entry_dir *dir, end = strchr(path, '\0'); name_len = end - path; + ret = node_enter(&dir->e); + if ( ret ) + return ERR_PTR(ret); + entry = dir->e.funcs->findentry(dir, path, name_len); if ( IS_ERR(entry) || !*end ) return entry; @@ -265,6 +330,7 @@ int hypfs_read_dir(const struct hypfs_entry *entry, const struct hypfs_entry_dir *d; const struct hypfs_entry *e; unsigned int size = entry->funcs->getsize(entry); + int ret; ASSERT(this_cpu(hypfs_locked) != hypfs_unlocked); @@ -276,12 +342,19 @@ int hypfs_read_dir(const struct hypfs_entry *entry, unsigned int e_namelen = strlen(e->name); unsigned int e_len = DIRENTRY_SIZE(e_namelen); + ret = node_enter(e); + if ( ret ) + return ret; + direntry.e.pad = 0; direntry.e.type = e->type; direntry.e.encoding = e->encoding; direntry.e.content_len = e->funcs->getsize(e); direntry.e.max_write_len = e->max_size; direntry.off_next = list_is_last(&e->list, &d->dirlist) ? 0 : e_len; + + node_exit(e); + if ( copy_to_guest(uaddr, &direntry, 1) ) return -EFAULT; @@ -490,6 +563,10 @@ long do_hypfs_op(unsigned int cmd, goto out; } + ret = node_enter(entry); + if ( ret ) + goto out; + switch ( cmd ) { case XEN_HYPFS_OP_read: @@ -506,6 +583,8 @@ long do_hypfs_op(unsigned int cmd, } out: + node_exit_all(); + hypfs_unlock(); return ret; diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h index 53f50772b4..8d96abd805 100644 --- a/xen/include/xen/hypfs.h +++ b/xen/include/xen/hypfs.h @@ -35,6 +35,8 @@ struct hypfs_entry; * "/a/b/c" findentry() will be called for "/", "/a", and "/a/b"). */ struct hypfs_funcs { + const struct hypfs_entry *(*enter)(const struct hypfs_entry *entry); + void (*exit)(const struct hypfs_entry *entry); int (*read)(const struct hypfs_entry *entry, XEN_GUEST_HANDLE_PARAM(void) uaddr); int (*write)(struct hypfs_entry_leaf *leaf, @@ -56,6 +58,7 @@ struct hypfs_entry { unsigned int size; unsigned int max_size; const char *name; + struct hypfs_entry *parent; struct list_head list; const struct hypfs_funcs *funcs; }; @@ -149,6 +152,8 @@ int hypfs_add_dir(struct hypfs_entry_dir *parent, struct hypfs_entry_dir *dir, bool nofault); int hypfs_add_leaf(struct hypfs_entry_dir *parent, struct hypfs_entry_leaf *leaf, bool nofault); +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry); +void hypfs_node_exit(const struct hypfs_entry *entry); int hypfs_read_dir(const struct hypfs_entry *entry, XEN_GUEST_HANDLE_PARAM(void) uaddr); int hypfs_read_leaf(const struct hypfs_entry *entry,
In order to better support resource allocation and locking for dynamic hypfs nodes add enter() and exit() callbacks to struct hypfs_funcs. The enter() callback is called when entering a node during hypfs user actions (traversing, reading or writing it), while the exit() callback is called when leaving a node (accessing another node at the same or a higher directory level, or when returning to the user). For avoiding recursion this requires a parent pointer in each node. Let the enter() callback return the entry address which is stored as the last accessed node in order to be able to use a template entry for that purpose in case of dynamic entries. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - new patch --- xen/common/hypfs.c | 79 +++++++++++++++++++++++++++++++++++++++++ xen/include/xen/hypfs.h | 5 +++ 2 files changed, 84 insertions(+)