| Message ID | 20201026091316.25680-10-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: support per-cpupool scheduling granularity | expand |
On 26.10.2020 10:13, Juergen Gross wrote: > --- a/xen/common/hypfs.c > +++ b/xen/common/hypfs.c > @@ -257,6 +257,82 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) > return entry->size; > } > > +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template, > + unsigned int id, bool is_last, > + XEN_GUEST_HANDLE_PARAM(void) *uaddr) > +{ > + struct xen_hypfs_dirlistentry direntry; > + char name[12]; Perhaps better tie this literal 12 to the one used for declaring struct hypfs_dyndir_id's name[] field, such that an eventual change will need making in exactly one place? > + unsigned int e_namelen, e_len; > + > + e_namelen = snprintf(name, sizeof(name), "%u", id); > + e_len = HYPFS_DIRENTRY_SIZE(e_namelen); > + direntry.e.pad = 0; > + direntry.e.type = template->e.type; > + direntry.e.encoding = template->e.encoding; > + direntry.e.content_len = template->e.funcs->getsize(&template->e); > + direntry.e.max_write_len = template->e.max_size; > + direntry.off_next = is_last ? 0 : e_len; > + if ( copy_to_guest(*uaddr, &direntry, 1) ) > + return -EFAULT; > + if ( copy_to_guest_offset(*uaddr, HYPFS_DIRENTRY_NAME_OFF, name, > + e_namelen + 1) ) > + return -EFAULT; > + > + guest_handle_add_offset(*uaddr, e_len); > + > + return 0; > +} > + > +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir, > + const char *name, > + unsigned int name_len) > +{ > + struct hypfs_dyndir_id *data; const? (also in read_dyndir below) > + data = hypfs_get_dyndata(); > + if ( !data ) > + return ERR_PTR(-ENOENT); > + > + /* Use template with original findentry function. */ > + return data->template->e.funcs->findentry(data->template, name, name_len); Why does this pass the address of the template? If it truly is (just) a template, then its dirlist ought to be empty at all times? If otoh the "template" indeed gets used as a node in the tree then perhaps it wants naming differently? "Stem" would come to mind, but likely there are better alternatives. I've also considered the German "Statthalter", but its English translations don't seem reasonable to me here. And "placeholder" has kind of a negative touch. (Also in this case some of my "const?" remarks may be irrelevant.) Further this and ... > +static int hypfs_read_dyndir(const struct hypfs_entry *entry, > + XEN_GUEST_HANDLE_PARAM(void) uaddr) > +{ > + struct hypfs_dyndir_id *data; > + > + data = hypfs_get_dyndata(); > + if ( !data ) > + return -ENOENT; > + > + /* Use template with original read function. */ > + return data->template->e.funcs->read(&data->template->e, uaddr); ... this using the template's funcs is somewhat unexpected, but with the functions acting as the entry's .findentry() / .read() hooks is obviously the right thing (and if the template is more that what the word says, the consideration may become inapplicable anyway). The implication is that the hooks themselves can't be replaced, if need be down the road. > +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template, > + unsigned int id) > +{ > + struct hypfs_dyndir_id *data; > + > + data = hypfs_alloc_dyndata(sizeof(*data), alignof(*data)); > + if ( !data ) > + return ERR_PTR(-ENOMEM); > + > + data->template = template; > + data->id = id; I can't seem to spot any consumer of this field: Is it really needed? > --- a/xen/include/xen/hypfs.h > +++ b/xen/include/xen/hypfs.h > @@ -50,6 +50,15 @@ struct hypfs_entry_dir { > struct list_head dirlist; > }; > > +struct hypfs_dyndir_id { > + struct hypfs_entry_dir dir; /* Modified copy of template. */ > + struct hypfs_funcs funcs; /* Dynamic functions. */ > + struct hypfs_entry_dir *template; /* Template used. */ const? > @@ -150,6 +159,11 @@ struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir, > unsigned int name_len); > void *hypfs_alloc_dyndata(unsigned long size, unsigned long align); > void *hypfs_get_dyndata(void); > +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template, const? > + unsigned int id, bool is_last, > + XEN_GUEST_HANDLE_PARAM(void) *uaddr); > +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template, const? Jan
On 17.11.20 14:33, Jan Beulich wrote: > On 26.10.2020 10:13, Juergen Gross wrote: >> --- a/xen/common/hypfs.c >> +++ b/xen/common/hypfs.c >> @@ -257,6 +257,82 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) >> return entry->size; >> } >> >> +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template, >> + unsigned int id, bool is_last, >> + XEN_GUEST_HANDLE_PARAM(void) *uaddr) >> +{ >> + struct xen_hypfs_dirlistentry direntry; >> + char name[12]; > > Perhaps better tie this literal 12 to the one used for declaring > struct hypfs_dyndir_id's name[] field, such that an eventual > change will need making in exactly one place? Yes. > >> + unsigned int e_namelen, e_len; >> + >> + e_namelen = snprintf(name, sizeof(name), "%u", id); >> + e_len = HYPFS_DIRENTRY_SIZE(e_namelen); >> + direntry.e.pad = 0; >> + direntry.e.type = template->e.type; >> + direntry.e.encoding = template->e.encoding; >> + direntry.e.content_len = template->e.funcs->getsize(&template->e); >> + direntry.e.max_write_len = template->e.max_size; >> + direntry.off_next = is_last ? 0 : e_len; >> + if ( copy_to_guest(*uaddr, &direntry, 1) ) >> + return -EFAULT; >> + if ( copy_to_guest_offset(*uaddr, HYPFS_DIRENTRY_NAME_OFF, name, >> + e_namelen + 1) ) >> + return -EFAULT; >> + >> + guest_handle_add_offset(*uaddr, e_len); >> + >> + return 0; >> +} >> + >> +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir, >> + const char *name, >> + unsigned int name_len) >> +{ >> + struct hypfs_dyndir_id *data; > > const? (also in read_dyndir below) Okay. > >> + data = hypfs_get_dyndata(); >> + if ( !data ) >> + return ERR_PTR(-ENOENT); >> + >> + /* Use template with original findentry function. */ >> + return data->template->e.funcs->findentry(data->template, name, name_len); > > Why does this pass the address of the template? If it truly is > (just) a template, then its dirlist ought to be empty at all > times? If otoh the "template" indeed gets used as a node in the > tree then perhaps it wants naming differently? "Stem" would come > to mind, but likely there are better alternatives. I've also > considered the German "Statthalter", but its English translations > don't seem reasonable to me here. And "placeholder" has kind of a > negative touch. (Also in this case some of my "const?" remarks > may be irrelevant.) It is basically a template tree. In the current use case (cpupool/<id>/sched-gran) the template is <id> with the leaf "sched-gran" which is the template for the per cpupool incarnation. If you like it better I can use stem. > > Further this and ... > >> +static int hypfs_read_dyndir(const struct hypfs_entry *entry, >> + XEN_GUEST_HANDLE_PARAM(void) uaddr) >> +{ >> + struct hypfs_dyndir_id *data; >> + >> + data = hypfs_get_dyndata(); >> + if ( !data ) >> + return -ENOENT; >> + >> + /* Use template with original read function. */ >> + return data->template->e.funcs->read(&data->template->e, uaddr); > > ... this using the template's funcs is somewhat unexpected, but > with the functions acting as the entry's .findentry() / .read() > hooks is obviously the right thing (and if the template is more > that what the word says, the consideration may become > inapplicable anyway). The implication is that the hooks > themselves can't be replaced, if need be down the road. Correct. In case this is needed the related node must be really completely dynamical instead. > >> +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template, >> + unsigned int id) >> +{ >> + struct hypfs_dyndir_id *data; >> + >> + data = hypfs_alloc_dyndata(sizeof(*data), alignof(*data)); >> + if ( !data ) >> + return ERR_PTR(-ENOMEM); >> + >> + data->template = template; >> + data->id = id; > > I can't seem to spot any consumer of this field: Is it really > needed? Yes. It will be used by the specific read/write functions, e.g. cpupool_gran_read(). > >> --- a/xen/include/xen/hypfs.h >> +++ b/xen/include/xen/hypfs.h >> @@ -50,6 +50,15 @@ struct hypfs_entry_dir { >> struct list_head dirlist; >> }; >> >> +struct hypfs_dyndir_id { >> + struct hypfs_entry_dir dir; /* Modified copy of template. */ >> + struct hypfs_funcs funcs; /* Dynamic functions. */ >> + struct hypfs_entry_dir *template; /* Template used. */ > > const? Yes. > >> @@ -150,6 +159,11 @@ struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir, >> unsigned int name_len); >> void *hypfs_alloc_dyndata(unsigned long size, unsigned long align); >> void *hypfs_get_dyndata(void); >> +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template, > > const? Yes. > >> + unsigned int id, bool is_last, >> + XEN_GUEST_HANDLE_PARAM(void) *uaddr); >> +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template, > > const? Yes. Juergen
On 17.11.2020 15:38, Jürgen Groß wrote: > On 17.11.20 14:33, Jan Beulich wrote: >> On 26.10.2020 10:13, Juergen Gross wrote: >>> +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir, >>> + const char *name, >>> + unsigned int name_len) >>> +{ >>> + struct hypfs_dyndir_id *data; >>> + >>> + data = hypfs_get_dyndata(); >>> + if ( !data ) >>> + return ERR_PTR(-ENOENT); >>> + >>> + /* Use template with original findentry function. */ >>> + return data->template->e.funcs->findentry(data->template, name, name_len); >> >> Why does this pass the address of the template? If it truly is >> (just) a template, then its dirlist ought to be empty at all >> times? If otoh the "template" indeed gets used as a node in the >> tree then perhaps it wants naming differently? "Stem" would come >> to mind, but likely there are better alternatives. I've also >> considered the German "Statthalter", but its English translations >> don't seem reasonable to me here. And "placeholder" has kind of a >> negative touch. (Also in this case some of my "const?" remarks >> may be irrelevant.) > > It is basically a template tree. > > In the current use case (cpupool/<id>/sched-gran) the template is > <id> with the leaf "sched-gran" which is the template for the per > cpupool incarnation. I can see sched-gran being a template, albeit even that will get dirtied as soon as a second leaf appears, as then these entries again need linking together. I think anything called template should be strictly r/o. It's also not clear to me how adding a 2nd level in the hierarchy would end up working: <component>/<id1>/<id2>/<leaf>. How would <leaf> know all the higher level IDs it's associated with? While I don't think this needs making work right away, the underlying model at least shouldn't prohibit it. Jan
On 17.11.20 15:50, Jan Beulich wrote: > On 17.11.2020 15:38, Jürgen Groß wrote: >> On 17.11.20 14:33, Jan Beulich wrote: >>> On 26.10.2020 10:13, Juergen Gross wrote: >>>> +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir, >>>> + const char *name, >>>> + unsigned int name_len) >>>> +{ >>>> + struct hypfs_dyndir_id *data; >>>> + >>>> + data = hypfs_get_dyndata(); >>>> + if ( !data ) >>>> + return ERR_PTR(-ENOENT); >>>> + >>>> + /* Use template with original findentry function. */ >>>> + return data->template->e.funcs->findentry(data->template, name, name_len); >>> >>> Why does this pass the address of the template? If it truly is >>> (just) a template, then its dirlist ought to be empty at all >>> times? If otoh the "template" indeed gets used as a node in the >>> tree then perhaps it wants naming differently? "Stem" would come >>> to mind, but likely there are better alternatives. I've also >>> considered the German "Statthalter", but its English translations >>> don't seem reasonable to me here. And "placeholder" has kind of a >>> negative touch. (Also in this case some of my "const?" remarks >>> may be irrelevant.) >> >> It is basically a template tree. >> >> In the current use case (cpupool/<id>/sched-gran) the template is >> <id> with the leaf "sched-gran" which is the template for the per >> cpupool incarnation. > > I can see sched-gran being a template, albeit even that will get > dirtied as soon as a second leaf appears, as then these entries > again need linking together. I think anything called template > should be strictly r/o. After boot the template will be constant, just like the hypfs tree created at boot time. In theory it could be setup completely static, but this would be a rather awful mess of macros. > It's also not clear to me how adding a 2nd level in the hierarchy > would end up working: <component>/<id1>/<id2>/<leaf>. How would > <leaf> know all the higher level IDs it's associated with? While > I don't think this needs making work right away, the underlying > model at least shouldn't prohibit it. This is the purpose of hypfs_alloc_dyndata(). You'd need something like struct hypfs_dyndir_id, but with two ids in it. Juergen
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c index 4c226a06b4..12be2f6d16 100644 --- a/xen/common/hypfs.c +++ b/xen/common/hypfs.c @@ -257,6 +257,82 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) return entry->size; } +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template, + unsigned int id, bool is_last, + XEN_GUEST_HANDLE_PARAM(void) *uaddr) +{ + struct xen_hypfs_dirlistentry direntry; + char name[12]; + unsigned int e_namelen, e_len; + + e_namelen = snprintf(name, sizeof(name), "%u", id); + e_len = HYPFS_DIRENTRY_SIZE(e_namelen); + direntry.e.pad = 0; + direntry.e.type = template->e.type; + direntry.e.encoding = template->e.encoding; + direntry.e.content_len = template->e.funcs->getsize(&template->e); + direntry.e.max_write_len = template->e.max_size; + direntry.off_next = is_last ? 0 : e_len; + if ( copy_to_guest(*uaddr, &direntry, 1) ) + return -EFAULT; + if ( copy_to_guest_offset(*uaddr, HYPFS_DIRENTRY_NAME_OFF, name, + e_namelen + 1) ) + return -EFAULT; + + guest_handle_add_offset(*uaddr, e_len); + + return 0; +} + +static struct hypfs_entry *hypfs_dyndir_findentry(struct hypfs_entry_dir *dir, + const char *name, + unsigned int name_len) +{ + struct hypfs_dyndir_id *data; + + data = hypfs_get_dyndata(); + if ( !data ) + return ERR_PTR(-ENOENT); + + /* Use template with original findentry function. */ + return data->template->e.funcs->findentry(data->template, name, name_len); +} + +static int hypfs_read_dyndir(const struct hypfs_entry *entry, + XEN_GUEST_HANDLE_PARAM(void) uaddr) +{ + struct hypfs_dyndir_id *data; + + data = hypfs_get_dyndata(); + if ( !data ) + return -ENOENT; + + /* Use template with original read function. */ + return data->template->e.funcs->read(&data->template->e, uaddr); +} + +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template, + unsigned int id) +{ + struct hypfs_dyndir_id *data; + + data = hypfs_alloc_dyndata(sizeof(*data), alignof(*data)); + if ( !data ) + return ERR_PTR(-ENOMEM); + + data->template = template; + data->id = id; + snprintf(data->name, sizeof(data->name), "%u", id); + data->dir = *template; + data->dir.e.name = data->name; + data->dir.e.funcs = &data->funcs; + data->funcs = *template->e.funcs; + data->funcs.findentry = hypfs_dyndir_findentry; + data->funcs.read = hypfs_read_dyndir; + + return &data->dir.e; +} + int hypfs_read_dir(const struct hypfs_entry *entry, XEN_GUEST_HANDLE_PARAM(void) uaddr) { diff --git a/xen/include/xen/hypfs.h b/xen/include/xen/hypfs.h index c8999b5381..adfb522496 100644 --- a/xen/include/xen/hypfs.h +++ b/xen/include/xen/hypfs.h @@ -50,6 +50,15 @@ struct hypfs_entry_dir { struct list_head dirlist; }; +struct hypfs_dyndir_id { + struct hypfs_entry_dir dir; /* Modified copy of template. */ + struct hypfs_funcs funcs; /* Dynamic functions. */ + struct hypfs_entry_dir *template; /* Template used. */ + char name[12]; /* Name of hypfs entry. */ + + unsigned int id; /* Numerical id. */ +}; + #define HYPFS_DIRENTRY_NAME_OFF offsetof(struct xen_hypfs_dirlistentry, name) #define HYPFS_DIRENTRY_SIZE(name_len) \ (HYPFS_DIRENTRY_NAME_OFF + \ @@ -150,6 +159,11 @@ struct hypfs_entry *hypfs_dir_findentry(struct hypfs_entry_dir *dir, unsigned int name_len); void *hypfs_alloc_dyndata(unsigned long size, unsigned long align); void *hypfs_get_dyndata(void); +int hypfs_read_dyndir_id_entry(struct hypfs_entry_dir *template, + unsigned int id, bool is_last, + XEN_GUEST_HANDLE_PARAM(void) *uaddr); +struct hypfs_entry *hypfs_gen_dyndir_entry_id(struct hypfs_entry_dir *template, + unsigned int id); #endif #endif /* __XEN_HYPFS_H__ */
Add some helpers to hypfs.c to support dynamic directories with a numerical id as name. The dynamic directory is based on a template specified by the user allowing to use specific access functions and having a predefined set of entries in the directory. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/hypfs.c | 76 +++++++++++++++++++++++++++++++++++++++++ xen/include/xen/hypfs.h | 14 ++++++++ 2 files changed, 90 insertions(+)