| Message ID | 20201201082128.15239-15-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: > --- a/xen/common/hypfs.c > +++ b/xen/common/hypfs.c > @@ -355,6 +355,81 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) > return entry->size; > } > > +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, > + unsigned int id, bool is_last, > + XEN_GUEST_HANDLE_PARAM(void) *uaddr) > +{ > + struct xen_hypfs_dirlistentry direntry; > + char name[HYPFS_DYNDIR_ID_NAMELEN]; > + unsigned int e_namelen, e_len; > + > + e_namelen = snprintf(name, sizeof(name), template->e.name, id); > + e_len = 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, 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( > + const struct hypfs_entry_dir *dir, const char *name, unsigned int name_len) > +{ > + const struct hypfs_dyndir_id *data; > + > + data = hypfs_get_dyndata(); > + > + /* 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) > +{ > + const struct hypfs_dyndir_id *data; > + > + data = hypfs_get_dyndata(); > + > + /* Use template with original read function. */ > + return data->template->e.funcs->read(&data->template->e, uaddr); > +} > + > +struct hypfs_entry *hypfs_gen_dyndir_entry_id( > + const struct hypfs_entry_dir *template, unsigned int id) > +{ > + struct hypfs_dyndir_id *data; > + > + data = hypfs_get_dyndata(); > + > + data->template = template; > + data->id = id; > + snprintf(data->name, sizeof(data->name), template->e.name, id); > + data->dir = *template; > + data->dir.e.name = data->name; I'm somewhat puzzled, if not confused, by the apparent redundancy of this name generation with that in hypfs_read_dyndir_id_entry(). Wasn't the idea to be able to use generic functions on these generated entries? Also, seeing that other function's name, wouldn't the one here want to be named hypfs_gen_dyndir_id_entry()? Jan
On 03.12.20 16:44, Jan Beulich wrote: > On 01.12.2020 09:21, Juergen Gross wrote: >> --- a/xen/common/hypfs.c >> +++ b/xen/common/hypfs.c >> @@ -355,6 +355,81 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) >> return entry->size; >> } >> >> +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, >> + unsigned int id, bool is_last, >> + XEN_GUEST_HANDLE_PARAM(void) *uaddr) >> +{ >> + struct xen_hypfs_dirlistentry direntry; >> + char name[HYPFS_DYNDIR_ID_NAMELEN]; >> + unsigned int e_namelen, e_len; >> + >> + e_namelen = snprintf(name, sizeof(name), template->e.name, id); >> + e_len = 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, 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( >> + const struct hypfs_entry_dir *dir, const char *name, unsigned int name_len) >> +{ >> + const struct hypfs_dyndir_id *data; >> + >> + data = hypfs_get_dyndata(); >> + >> + /* 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) >> +{ >> + const struct hypfs_dyndir_id *data; >> + >> + data = hypfs_get_dyndata(); >> + >> + /* Use template with original read function. */ >> + return data->template->e.funcs->read(&data->template->e, uaddr); >> +} >> + >> +struct hypfs_entry *hypfs_gen_dyndir_entry_id( >> + const struct hypfs_entry_dir *template, unsigned int id) >> +{ >> + struct hypfs_dyndir_id *data; >> + >> + data = hypfs_get_dyndata(); >> + >> + data->template = template; >> + data->id = id; >> + snprintf(data->name, sizeof(data->name), template->e.name, id); >> + data->dir = *template; >> + data->dir.e.name = data->name; > > I'm somewhat puzzled, if not confused, by the apparent redundancy > of this name generation with that in hypfs_read_dyndir_id_entry(). > Wasn't the idea to be able to use generic functions on these > generated entries? I can add a macro replacing the double snprintf(). > Also, seeing that other function's name, wouldn't the one here > want to be named hypfs_gen_dyndir_id_entry()? Fine with me. Juergen
On 04.12.2020 09:52, Jürgen Groß wrote: > On 03.12.20 16:44, Jan Beulich wrote: >> On 01.12.2020 09:21, Juergen Gross wrote: >>> --- a/xen/common/hypfs.c >>> +++ b/xen/common/hypfs.c >>> @@ -355,6 +355,81 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) >>> return entry->size; >>> } >>> >>> +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, >>> + unsigned int id, bool is_last, >>> + XEN_GUEST_HANDLE_PARAM(void) *uaddr) >>> +{ >>> + struct xen_hypfs_dirlistentry direntry; >>> + char name[HYPFS_DYNDIR_ID_NAMELEN]; >>> + unsigned int e_namelen, e_len; >>> + >>> + e_namelen = snprintf(name, sizeof(name), template->e.name, id); >>> + e_len = 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, 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( >>> + const struct hypfs_entry_dir *dir, const char *name, unsigned int name_len) >>> +{ >>> + const struct hypfs_dyndir_id *data; >>> + >>> + data = hypfs_get_dyndata(); >>> + >>> + /* 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) >>> +{ >>> + const struct hypfs_dyndir_id *data; >>> + >>> + data = hypfs_get_dyndata(); >>> + >>> + /* Use template with original read function. */ >>> + return data->template->e.funcs->read(&data->template->e, uaddr); >>> +} >>> + >>> +struct hypfs_entry *hypfs_gen_dyndir_entry_id( >>> + const struct hypfs_entry_dir *template, unsigned int id) >>> +{ >>> + struct hypfs_dyndir_id *data; >>> + >>> + data = hypfs_get_dyndata(); >>> + >>> + data->template = template; >>> + data->id = id; >>> + snprintf(data->name, sizeof(data->name), template->e.name, id); >>> + data->dir = *template; >>> + data->dir.e.name = data->name; >> >> I'm somewhat puzzled, if not confused, by the apparent redundancy >> of this name generation with that in hypfs_read_dyndir_id_entry(). >> Wasn't the idea to be able to use generic functions on these >> generated entries? > > I can add a macro replacing the double snprintf(). That wasn't my point. I'm concerned of there being two name generation sites in the first place. Is this perhaps simply some form of optimization, avoiding hypfs_read_dyndir_id_entry() to call hypfs_gen_dyndir_entry_id() (but risking the two going out of sync)? Jan
On 04.12.20 10:16, Jan Beulich wrote: > On 04.12.2020 09:52, Jürgen Groß wrote: >> On 03.12.20 16:44, Jan Beulich wrote: >>> On 01.12.2020 09:21, Juergen Gross wrote: >>>> --- a/xen/common/hypfs.c >>>> +++ b/xen/common/hypfs.c >>>> @@ -355,6 +355,81 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) >>>> return entry->size; >>>> } >>>> >>>> +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, >>>> + unsigned int id, bool is_last, >>>> + XEN_GUEST_HANDLE_PARAM(void) *uaddr) >>>> +{ >>>> + struct xen_hypfs_dirlistentry direntry; >>>> + char name[HYPFS_DYNDIR_ID_NAMELEN]; >>>> + unsigned int e_namelen, e_len; >>>> + >>>> + e_namelen = snprintf(name, sizeof(name), template->e.name, id); >>>> + e_len = 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, 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( >>>> + const struct hypfs_entry_dir *dir, const char *name, unsigned int name_len) >>>> +{ >>>> + const struct hypfs_dyndir_id *data; >>>> + >>>> + data = hypfs_get_dyndata(); >>>> + >>>> + /* 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) >>>> +{ >>>> + const struct hypfs_dyndir_id *data; >>>> + >>>> + data = hypfs_get_dyndata(); >>>> + >>>> + /* Use template with original read function. */ >>>> + return data->template->e.funcs->read(&data->template->e, uaddr); >>>> +} >>>> + >>>> +struct hypfs_entry *hypfs_gen_dyndir_entry_id( >>>> + const struct hypfs_entry_dir *template, unsigned int id) >>>> +{ >>>> + struct hypfs_dyndir_id *data; >>>> + >>>> + data = hypfs_get_dyndata(); >>>> + >>>> + data->template = template; >>>> + data->id = id; >>>> + snprintf(data->name, sizeof(data->name), template->e.name, id); >>>> + data->dir = *template; >>>> + data->dir.e.name = data->name; >>> >>> I'm somewhat puzzled, if not confused, by the apparent redundancy >>> of this name generation with that in hypfs_read_dyndir_id_entry(). >>> Wasn't the idea to be able to use generic functions on these >>> generated entries? >> >> I can add a macro replacing the double snprintf(). > > That wasn't my point. I'm concerned of there being two name generation > sites in the first place. Is this perhaps simply some form of > optimization, avoiding hypfs_read_dyndir_id_entry() to call > hypfs_gen_dyndir_entry_id() (but risking the two going out of sync)? Be aware that hypfs_read_dyndir_id_entry() is generating a struct xen_hypfs_dirlistentry, which is different from the internal representation of the data produced by hypfs_gen_dyndir_entry_id(). So the main common part is the name generation. What else would you want apart from making it common via e.g. a macro? Letting hypfs_read_dyndir_id_entry() call hypfs_gen_dyndir_entry_id() would just be a more general approach with all the data but the generated name of hypfs_gen_dyndir_entry_id() dropped or just copied around a second time. Juergen
On 04.12.2020 14:08, Jürgen Groß wrote: > On 04.12.20 10:16, Jan Beulich wrote: >> On 04.12.2020 09:52, Jürgen Groß wrote: >>> On 03.12.20 16:44, Jan Beulich wrote: >>>> On 01.12.2020 09:21, Juergen Gross wrote: >>>>> --- a/xen/common/hypfs.c >>>>> +++ b/xen/common/hypfs.c >>>>> @@ -355,6 +355,81 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) >>>>> return entry->size; >>>>> } >>>>> >>>>> +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, >>>>> + unsigned int id, bool is_last, >>>>> + XEN_GUEST_HANDLE_PARAM(void) *uaddr) >>>>> +{ >>>>> + struct xen_hypfs_dirlistentry direntry; >>>>> + char name[HYPFS_DYNDIR_ID_NAMELEN]; >>>>> + unsigned int e_namelen, e_len; >>>>> + >>>>> + e_namelen = snprintf(name, sizeof(name), template->e.name, id); >>>>> + e_len = 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, 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( >>>>> + const struct hypfs_entry_dir *dir, const char *name, unsigned int name_len) >>>>> +{ >>>>> + const struct hypfs_dyndir_id *data; >>>>> + >>>>> + data = hypfs_get_dyndata(); >>>>> + >>>>> + /* 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) >>>>> +{ >>>>> + const struct hypfs_dyndir_id *data; >>>>> + >>>>> + data = hypfs_get_dyndata(); >>>>> + >>>>> + /* Use template with original read function. */ >>>>> + return data->template->e.funcs->read(&data->template->e, uaddr); >>>>> +} >>>>> + >>>>> +struct hypfs_entry *hypfs_gen_dyndir_entry_id( >>>>> + const struct hypfs_entry_dir *template, unsigned int id) >>>>> +{ >>>>> + struct hypfs_dyndir_id *data; >>>>> + >>>>> + data = hypfs_get_dyndata(); >>>>> + >>>>> + data->template = template; >>>>> + data->id = id; >>>>> + snprintf(data->name, sizeof(data->name), template->e.name, id); >>>>> + data->dir = *template; >>>>> + data->dir.e.name = data->name; >>>> >>>> I'm somewhat puzzled, if not confused, by the apparent redundancy >>>> of this name generation with that in hypfs_read_dyndir_id_entry(). >>>> Wasn't the idea to be able to use generic functions on these >>>> generated entries? >>> >>> I can add a macro replacing the double snprintf(). >> >> That wasn't my point. I'm concerned of there being two name generation >> sites in the first place. Is this perhaps simply some form of >> optimization, avoiding hypfs_read_dyndir_id_entry() to call >> hypfs_gen_dyndir_entry_id() (but risking the two going out of sync)? > > Be aware that hypfs_read_dyndir_id_entry() is generating a struct > xen_hypfs_dirlistentry, which is different from the internal > representation of the data produced by hypfs_gen_dyndir_entry_id(). > > So the main common part is the name generation. What else would you > want apart from making it common via e.g. a macro? Letting > hypfs_read_dyndir_id_entry() call hypfs_gen_dyndir_entry_id() would > just be a more general approach with all the data but the generated > name of hypfs_gen_dyndir_entry_id() dropped or just copied around > a second time. IOW just an optimization, as I was assuming. Whether you macroize the name generation I'd like to leave up to you. But you could please add comments on both sides as to parts which need to remain in sync? Jan
diff --git a/xen/common/hypfs.c b/xen/common/hypfs.c index 39448c5409..1819b26925 100644 --- a/xen/common/hypfs.c +++ b/xen/common/hypfs.c @@ -355,6 +355,81 @@ unsigned int hypfs_getsize(const struct hypfs_entry *entry) return entry->size; } +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, + unsigned int id, bool is_last, + XEN_GUEST_HANDLE_PARAM(void) *uaddr) +{ + struct xen_hypfs_dirlistentry direntry; + char name[HYPFS_DYNDIR_ID_NAMELEN]; + unsigned int e_namelen, e_len; + + e_namelen = snprintf(name, sizeof(name), template->e.name, id); + e_len = 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, 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( + const struct hypfs_entry_dir *dir, const char *name, unsigned int name_len) +{ + const struct hypfs_dyndir_id *data; + + data = hypfs_get_dyndata(); + + /* 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) +{ + const struct hypfs_dyndir_id *data; + + data = hypfs_get_dyndata(); + + /* Use template with original read function. */ + return data->template->e.funcs->read(&data->template->e, uaddr); +} + +struct hypfs_entry *hypfs_gen_dyndir_entry_id( + const struct hypfs_entry_dir *template, unsigned int id) +{ + struct hypfs_dyndir_id *data; + + data = hypfs_get_dyndata(); + + data->template = template; + data->id = id; + snprintf(data->name, sizeof(data->name), template->e.name, 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; +} + +unsigned int hypfs_dynid_entry_size(const struct hypfs_entry *template, + unsigned int id) +{ + return DIRENTRY_SIZE(snprintf(NULL, 0, template->name, id)); +} + 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 be8d6c508a..1782c50b30 100644 --- a/xen/include/xen/hypfs.h +++ b/xen/include/xen/hypfs.h @@ -76,6 +76,16 @@ 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. */ + const struct hypfs_entry_dir *template; /* Template used. */ +#define HYPFS_DYNDIR_ID_NAMELEN 12 + char name[HYPFS_DYNDIR_ID_NAMELEN]; /* Name of hypfs entry. */ + + unsigned int id; /* Numerical id. */ +}; + #define HYPFS_VARDIR_INIT(var, nam, fn) \ struct hypfs_entry_dir __read_mostly var = { \ .e.type = XEN_HYPFS_TYPE_DIR, \ @@ -181,6 +191,13 @@ struct hypfs_entry *hypfs_dir_findentry(const struct hypfs_entry_dir *dir, void *hypfs_alloc_dyndata(unsigned long size); void *hypfs_get_dyndata(void); void hypfs_free_dyndata(void); +int hypfs_read_dyndir_id_entry(const 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( + const struct hypfs_entry_dir *template, unsigned int id); +unsigned int hypfs_dynid_entry_size(const struct hypfs_entry *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> --- V2: - use macro for length of entry name (Jan Beulich) - const attributes (Jan Beulich) - use template name as format string (Jan Beulich) - add hypfs_dynid_entry_size() helper (Jan Beulich) - expect dyndir data having been allocated by enter() callback --- xen/common/hypfs.c | 75 +++++++++++++++++++++++++++++++++++++++++ xen/include/xen/hypfs.h | 17 ++++++++++ 2 files changed, 92 insertions(+)