| Message ID | 20210324010547.4134370-1-minchan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] mm: cma: support sysfs | expand |
On 3/23/21 6:05 PM, Minchan Kim wrote: ...> diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c > new file mode 100644 > index 000000000000..c3791a032dc5 > --- /dev/null > +++ b/mm/cma_sysfs.c > @@ -0,0 +1,107 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * CMA SysFS Interface > + * > + * Copyright (c) 2021 Minchan Kim <minchan@kernel.org> > + */ > + > +#include <linux/cma.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > + > +#include "cma.h" > + > +void cma_sysfs_account_success_pages(struct cma *cma, size_t count) > +{ > + atomic64_add(count, &cma->nr_pages_succeeded); > +} > + > +void cma_sysfs_account_fail_pages(struct cma *cma, size_t count) > +{ > + atomic64_add(count, &cma->nr_pages_failed); > +} > + > +#define CMA_ATTR_RO(_name) \ > + static struct kobj_attribute _name##_attr = __ATTR_RO(_name) > + > +#define to_cma_kobject(x) container_of(x, struct cma_kobject, kobj) I really don't think that helps. container_of() is so widely used and understood that it is not a good move make people read one more wrapper for it. Instead, see below... > + > +static ssize_t alloc_pages_success_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct cma_kobject *cma_kobj = to_cma_kobject(kobj); > + struct cma *cma = cma_kobj->cma; ...if you're looking to get rid of the real code duplication, then you could put *both* of those lines into a wrapper function, instead, like this: static inline struct cma* cma_from_kobj(struct kobject *kobj) { struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject, kobj); struct cma *cma = cma_kobj->cma; return cma; } static ssize_t alloc_pages_success_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) { struct cma *cma = cma_from_kobj(kobj); return sysfs_emit(buf, "%llu\n", atomic64_read(&cma->nr_pages_succeeded)); } CMA_ATTR_RO(alloc_pages_success); static ssize_t alloc_pages_fail_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) { struct cma *cma = cma_from_kobj(kobj); return sysfs_emit(buf, "%llu\n", atomic64_read(&cma->nr_pages_failed)); } CMA_ATTR_RO(alloc_pages_fail); static void cma_kobj_release(struct kobject *kobj) { struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject, kobj); struct cma *cma = cma_kobj->cma; kfree(cma_kobj); cma->kobj = NULL; } ...isn't that nicer? Saves a little code, gets rid of a macro. > + > + return sysfs_emit(buf, "%llu\n", > + atomic64_read(&cma->nr_pages_succeeded)); > +} > +CMA_ATTR_RO(alloc_pages_success); > + > +static ssize_t alloc_pages_fail_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct cma_kobject *cma_kobj = to_cma_kobject(kobj); > + struct cma *cma = cma_kobj->cma; > + > + return sysfs_emit(buf, "%llu\n", atomic64_read(&cma->nr_pages_failed)); > +} > +CMA_ATTR_RO(alloc_pages_fail); > + > +static void cma_kobj_release(struct kobject *kobj) > +{ > + struct cma_kobject *cma_kobj = to_cma_kobject(kobj); > + struct cma *cma = cma_kobj->cma; > + > + kfree(cma_kobj); > + cma->kobj = NULL; > +} > + > +static struct attribute *cma_attrs[] = { > + &alloc_pages_success_attr.attr, > + &alloc_pages_fail_attr.attr, > + NULL, > +}; > +ATTRIBUTE_GROUPS(cma); > + > +static struct kobject *cma_kobj_root; > + > +static struct kobj_type cma_ktype = { > + .release = cma_kobj_release, > + .sysfs_ops = &kobj_sysfs_ops, > + .default_groups = cma_groups > +}; > + > +static int __init cma_sysfs_init(void) > +{ > + unsigned int i; > + > + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > + if (!cma_kobj_root) > + return -ENOMEM; > + > + for (i = 0; i < cma_area_count; i++) { > + int err; > + struct cma *cma; > + struct cma_kobject *cma_kobj; > + > + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > + if (!cma_kobj) { > + kobject_put(cma_kobj_root); > + return -ENOMEM; This leaks little cma_kobj's all over the floor. :) What you might want here is a separate routine to clean up, because it has to loop through and free whatever was allocated on previous iterations of this loop here. > + } > + > + cma = &cma_areas[i]; > + cma->kobj = cma_kobj; > + cma_kobj->cma = cma; > + err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype, > + cma_kobj_root, "%s", cma->name); > + if (err) { > + kobject_put(&cma_kobj->kobj); > + kobject_put(cma_kobj_root); > + return err; Hopefully this little bit of logic could also go into the cleanup routine. > + } > + } > + > + return 0; > +} > +subsys_initcall(cma_sysfs_init); > thanks,
On Tue, Mar 23, 2021 at 07:34:12PM -0700, John Hubbard wrote: > On 3/23/21 6:05 PM, Minchan Kim wrote: > ...> diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c > > new file mode 100644 > > index 000000000000..c3791a032dc5 > > --- /dev/null > > +++ b/mm/cma_sysfs.c > > @@ -0,0 +1,107 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * CMA SysFS Interface > > + * > > + * Copyright (c) 2021 Minchan Kim <minchan@kernel.org> > > + */ > > + > > +#include <linux/cma.h> > > +#include <linux/kernel.h> > > +#include <linux/slab.h> > > + > > +#include "cma.h" > > + > > +void cma_sysfs_account_success_pages(struct cma *cma, size_t count) > > +{ > > + atomic64_add(count, &cma->nr_pages_succeeded); > > +} > > + > > +void cma_sysfs_account_fail_pages(struct cma *cma, size_t count) > > +{ > > + atomic64_add(count, &cma->nr_pages_failed); > > +} > > + > > +#define CMA_ATTR_RO(_name) \ > > + static struct kobj_attribute _name##_attr = __ATTR_RO(_name) > > + > > +#define to_cma_kobject(x) container_of(x, struct cma_kobject, kobj) > > I really don't think that helps. container_of() is so widely used and > understood that it is not a good move make people read one more wrapper > for it. Instead, see below... > > > + > > +static ssize_t alloc_pages_success_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf) > > +{ > > + struct cma_kobject *cma_kobj = to_cma_kobject(kobj); > > + struct cma *cma = cma_kobj->cma; > > ...if you're looking to get rid of the real code duplication, then you > could put *both* of those lines into a wrapper function, instead, like this: > > static inline struct cma* cma_from_kobj(struct kobject *kobj) > { > struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject, > kobj); > struct cma *cma = cma_kobj->cma; > > return cma; > } > > static ssize_t alloc_pages_success_show(struct kobject *kobj, > struct kobj_attribute *attr, char *buf) > { > struct cma *cma = cma_from_kobj(kobj); > > return sysfs_emit(buf, "%llu\n", > atomic64_read(&cma->nr_pages_succeeded)); > } > CMA_ATTR_RO(alloc_pages_success); > > static ssize_t alloc_pages_fail_show(struct kobject *kobj, > struct kobj_attribute *attr, char *buf) > { > struct cma *cma = cma_from_kobj(kobj); > > return sysfs_emit(buf, "%llu\n", atomic64_read(&cma->nr_pages_failed)); > } > CMA_ATTR_RO(alloc_pages_fail); > > static void cma_kobj_release(struct kobject *kobj) > { > struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject, > kobj); > struct cma *cma = cma_kobj->cma; > > kfree(cma_kobj); > cma->kobj = NULL; > } > > ...isn't that nicer? Saves a little code, gets rid of a macro. Yub. > > > + > > + return sysfs_emit(buf, "%llu\n", > > + atomic64_read(&cma->nr_pages_succeeded)); > > +} > > +CMA_ATTR_RO(alloc_pages_success); > > + > > +static ssize_t alloc_pages_fail_show(struct kobject *kobj, > > + struct kobj_attribute *attr, char *buf) > > +{ > > + struct cma_kobject *cma_kobj = to_cma_kobject(kobj); > > + struct cma *cma = cma_kobj->cma; > > + > > + return sysfs_emit(buf, "%llu\n", atomic64_read(&cma->nr_pages_failed)); > > +} > > +CMA_ATTR_RO(alloc_pages_fail); > > + > > +static void cma_kobj_release(struct kobject *kobj) > > +{ > > + struct cma_kobject *cma_kobj = to_cma_kobject(kobj); > > + struct cma *cma = cma_kobj->cma; > > + > > + kfree(cma_kobj); > > + cma->kobj = NULL; > > +} > > + > > +static struct attribute *cma_attrs[] = { > > + &alloc_pages_success_attr.attr, > > + &alloc_pages_fail_attr.attr, > > + NULL, > > +}; > > +ATTRIBUTE_GROUPS(cma); > > + > > +static struct kobject *cma_kobj_root; > > + > > +static struct kobj_type cma_ktype = { > > + .release = cma_kobj_release, > > + .sysfs_ops = &kobj_sysfs_ops, > > + .default_groups = cma_groups > > +}; > > + > > +static int __init cma_sysfs_init(void) > > +{ > > + unsigned int i; > > + > > + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > > + if (!cma_kobj_root) > > + return -ENOMEM; > > + > > + for (i = 0; i < cma_area_count; i++) { > > + int err; > > + struct cma *cma; > > + struct cma_kobject *cma_kobj; > > + > > + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > > + if (!cma_kobj) { > > + kobject_put(cma_kobj_root); > > + return -ENOMEM; > > This leaks little cma_kobj's all over the floor. :) I thought kobject_put(cma_kobj_root) should deal with it. No? > > What you might want here is a separate routine to clean up, because > it has to loop through and free whatever was allocated on previous > iterations of this loop here. > > > + } > > + > > + cma = &cma_areas[i]; > > + cma->kobj = cma_kobj; > > + cma_kobj->cma = cma; > > + err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype, > > + cma_kobj_root, "%s", cma->name); > > + if (err) { > > + kobject_put(&cma_kobj->kobj); > > + kobject_put(cma_kobj_root); > > + return err; > > Hopefully this little bit of logic could also go into the cleanup > routine. > > > + } > > + } > > + > > + return 0; > > +} > > +subsys_initcall(cma_sysfs_init); > > > > thanks, > -- > John Hubbard > NVIDIA
On 3/23/21 8:27 PM, Minchan Kim wrote: ... >>> +static int __init cma_sysfs_init(void) >>> +{ >>> + unsigned int i; >>> + >>> + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); >>> + if (!cma_kobj_root) >>> + return -ENOMEM; >>> + >>> + for (i = 0; i < cma_area_count; i++) { >>> + int err; >>> + struct cma *cma; >>> + struct cma_kobject *cma_kobj; >>> + >>> + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); >>> + if (!cma_kobj) { >>> + kobject_put(cma_kobj_root); >>> + return -ENOMEM; >> >> This leaks little cma_kobj's all over the floor. :) > > I thought kobject_put(cma_kobj_root) should deal with it. No? > If this fails when i > 0, there will be cma_kobj instances that were stashed in the cma_areas[] array. But this code only deletes the most recently allocated cma_kobj, not anything allocated on previous iterations of the loop. thanks,
On Tue, Mar 23, 2021 at 09:47:27PM -0700, John Hubbard wrote: > On 3/23/21 8:27 PM, Minchan Kim wrote: > ... > > > > +static int __init cma_sysfs_init(void) > > > > +{ > > > > + unsigned int i; > > > > + > > > > + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > > > > + if (!cma_kobj_root) > > > > + return -ENOMEM; > > > > + > > > > + for (i = 0; i < cma_area_count; i++) { > > > > + int err; > > > > + struct cma *cma; > > > > + struct cma_kobject *cma_kobj; > > > > + > > > > + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > > > > + if (!cma_kobj) { > > > > + kobject_put(cma_kobj_root); > > > > + return -ENOMEM; > > > > > > This leaks little cma_kobj's all over the floor. :) > > > > I thought kobject_put(cma_kobj_root) should deal with it. No? > > > If this fails when i > 0, there will be cma_kobj instances that > were stashed in the cma_areas[] array. But this code only deletes > the most recently allocated cma_kobj, not anything allocated on > previous iterations of the loop. Oh, I misunderstood that destroying of root kobject will release children recursively. Seems not true. Go back to old version. index 16c81c9cb9b7..418951a3f138 100644 --- a/mm/cma_sysfs.c +++ b/mm/cma_sysfs.c @@ -80,20 +80,19 @@ static struct kobj_type cma_ktype = { static int __init cma_sysfs_init(void) { unsigned int i; + int err; + struct cma *cma; + struct cma_kobject *cma_kobj; cma_kobj_root = kobject_create_and_add("cma", mm_kobj); if (!cma_kobj_root) return -ENOMEM; for (i = 0; i < cma_area_count; i++) { - int err; - struct cma *cma; - struct cma_kobject *cma_kobj; - cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); if (!cma_kobj) { - kobject_put(cma_kobj_root); - return -ENOMEM; + err = -ENOMEM; + goto out; } cma = &cma_areas[i]; @@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void) cma_kobj_root, "%s", cma->name); if (err) { kobject_put(&cma_kobj->kobj); - kobject_put(cma_kobj_root); - return err; + goto out; } } return 0; +out: + while (--i >= 0) { + cma = &cma_areas[i]; + + kobject_put(&cma->kobj->kobj); + kfree(cma->kobj); + cma->kobj = NULL; + } + kobject_put(cma_kobj_root); + + return err; } subsys_initcall(cma_sysfs_init);
On 3/23/21 10:44 PM, Minchan Kim wrote: > On Tue, Mar 23, 2021 at 09:47:27PM -0700, John Hubbard wrote: >> On 3/23/21 8:27 PM, Minchan Kim wrote: >> ... >>>>> +static int __init cma_sysfs_init(void) >>>>> +{ >>>>> + unsigned int i; >>>>> + >>>>> + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); >>>>> + if (!cma_kobj_root) >>>>> + return -ENOMEM; >>>>> + >>>>> + for (i = 0; i < cma_area_count; i++) { >>>>> + int err; >>>>> + struct cma *cma; >>>>> + struct cma_kobject *cma_kobj; >>>>> + >>>>> + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); >>>>> + if (!cma_kobj) { >>>>> + kobject_put(cma_kobj_root); >>>>> + return -ENOMEM; >>>> >>>> This leaks little cma_kobj's all over the floor. :) >>> >>> I thought kobject_put(cma_kobj_root) should deal with it. No? >>> >> If this fails when i > 0, there will be cma_kobj instances that >> were stashed in the cma_areas[] array. But this code only deletes >> the most recently allocated cma_kobj, not anything allocated on >> previous iterations of the loop. > > Oh, I misunderstood that destroying of root kobject will release > children recursively. Seems not true. Go back to old version. > > > index 16c81c9cb9b7..418951a3f138 100644 > --- a/mm/cma_sysfs.c > +++ b/mm/cma_sysfs.c > @@ -80,20 +80,19 @@ static struct kobj_type cma_ktype = { > static int __init cma_sysfs_init(void) > { > unsigned int i; > + int err; > + struct cma *cma; > + struct cma_kobject *cma_kobj; > > cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > if (!cma_kobj_root) > return -ENOMEM; > > for (i = 0; i < cma_area_count; i++) { > - int err; > - struct cma *cma; > - struct cma_kobject *cma_kobj; > - > cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > if (!cma_kobj) { > - kobject_put(cma_kobj_root); > - return -ENOMEM; > + err = -ENOMEM; > + goto out; > } > > cma = &cma_areas[i]; > @@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void) > cma_kobj_root, "%s", cma->name); > if (err) { > kobject_put(&cma_kobj->kobj); > - kobject_put(cma_kobj_root); > - return err; > + goto out; > } > } > > return 0; > +out: > + while (--i >= 0) { > + cma = &cma_areas[i]; > + > + kobject_put(&cma->kobj->kobj); OK. As long as you are spinning a new version, let's fix up the naming to be a little better, too. In this case, with a mildly dizzying mix of cma's and kobjects, it actually makes a real difference. I wouldn't have asked, but the above cma->kobj->kobj chain really made it obvious for me just now. So instead of this (in cma.h): struct cma_kobject { struct cma *cma; struct kobject kobj; }; struct cma { ... struct cma_kobject *kobj; }; , how about approximately this: struct cma_kobject_wrapper { struct cma *parent; struct kobject kobj; }; struct cma { ... struct cma_kobject_wrapper *cma_kobj_wrapper; }; ...thus allowing readers of cma_sysfs.c to read that file more easily. > + kfree(cma->kobj); > + cma->kobj = NULL; > + } > + kobject_put(cma_kobj_root); > + > + return err; > } > subsys_initcall(cma_sysfs_init); > > > thanks,
On Tue, Mar 23, 2021 at 11:26:08PM -0700, John Hubbard wrote: > On 3/23/21 10:44 PM, Minchan Kim wrote: > > On Tue, Mar 23, 2021 at 09:47:27PM -0700, John Hubbard wrote: > > > On 3/23/21 8:27 PM, Minchan Kim wrote: > > > ... > > > > > > +static int __init cma_sysfs_init(void) > > > > > > +{ > > > > > > + unsigned int i; > > > > > > + > > > > > > + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > > > > > > + if (!cma_kobj_root) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + for (i = 0; i < cma_area_count; i++) { > > > > > > + int err; > > > > > > + struct cma *cma; > > > > > > + struct cma_kobject *cma_kobj; > > > > > > + > > > > > > + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > > > > > > + if (!cma_kobj) { > > > > > > + kobject_put(cma_kobj_root); > > > > > > + return -ENOMEM; > > > > > > > > > > This leaks little cma_kobj's all over the floor. :) > > > > > > > > I thought kobject_put(cma_kobj_root) should deal with it. No? > > > > > > > If this fails when i > 0, there will be cma_kobj instances that > > > were stashed in the cma_areas[] array. But this code only deletes > > > the most recently allocated cma_kobj, not anything allocated on > > > previous iterations of the loop. > > > > Oh, I misunderstood that destroying of root kobject will release > > children recursively. Seems not true. Go back to old version. > > > > > > index 16c81c9cb9b7..418951a3f138 100644 > > --- a/mm/cma_sysfs.c > > +++ b/mm/cma_sysfs.c > > @@ -80,20 +80,19 @@ static struct kobj_type cma_ktype = { > > static int __init cma_sysfs_init(void) > > { > > unsigned int i; > > + int err; > > + struct cma *cma; > > + struct cma_kobject *cma_kobj; > > > > cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > > if (!cma_kobj_root) > > return -ENOMEM; > > > > for (i = 0; i < cma_area_count; i++) { > > - int err; > > - struct cma *cma; > > - struct cma_kobject *cma_kobj; > > - > > cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > > if (!cma_kobj) { > > - kobject_put(cma_kobj_root); > > - return -ENOMEM; > > + err = -ENOMEM; > > + goto out; > > } > > > > cma = &cma_areas[i]; > > @@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void) > > cma_kobj_root, "%s", cma->name); > > if (err) { > > kobject_put(&cma_kobj->kobj); > > - kobject_put(cma_kobj_root); > > - return err; > > + goto out; > > } > > } > > > > return 0; > > +out: > > + while (--i >= 0) { > > + cma = &cma_areas[i]; > > + > > + kobject_put(&cma->kobj->kobj); > > > OK. As long as you are spinning a new version, let's fix up the naming to > be a little better, too. In this case, with a mildly dizzying mix of cma's > and kobjects, it actually makes a real difference. I wouldn't have asked, > but the above cma->kobj->kobj chain really made it obvious for me just now. > > So instead of this (in cma.h): > > struct cma_kobject { > struct cma *cma; > struct kobject kobj; > }; > > struct cma { > ... > struct cma_kobject *kobj; > }; > > , how about approximately this: > > struct cma_kobject_wrapper { > struct cma *parent; > struct kobject kobj; > }; > > struct cma { > ... > struct cma_kobject_wrapper *cma_kobj_wrapper; > }; > > > ...thus allowing readers of cma_sysfs.c to read that file more easily. I agree cma->kobj->kobj is awkward but personally, I don't like the naming: cma_kobject_wrapper parent pointer. cma_kobject is alredy wrapper so it sounds me redundant and it's not a parent in same hierarchy. Since the kobj->kobj is just one line in the code(I don't imagine it could grow up in cma_sysfs in future), I don't think it would be a problem. If we really want to make it more clear, maybe? cma->cma_kobj->kobj It would be consistent with other variables in cma_sysfs_init.
On 3/23/21 11:57 PM, Minchan Kim wrote: ... >> , how about approximately this: >> >> struct cma_kobject_wrapper { >> struct cma *parent; >> struct kobject kobj; >> }; >> >> struct cma { >> ... >> struct cma_kobject_wrapper *cma_kobj_wrapper; >> }; >> >> >> ...thus allowing readers of cma_sysfs.c to read that file more easily. > > I agree cma->kobj->kobj is awkward but personally, I don't like the > naming: cma_kobject_wrapper parent pointer. cma_kobject is alredy > wrapper so it sounds me redundant and it's not a parent in same > hierarchy. > > Since the kobj->kobj is just one line in the code(I don't imagine > it could grow up in cma_sysfs in future), I don't think it would > be a problem. If we really want to make it more clear, maybe? > > cma->cma_kobj->kobj > > It would be consistent with other variables in cma_sysfs_init. > OK, that's at least better than it was. thanks,
24.03.2021 04:05, Minchan Kim пишет: > +static struct kobject *cma_kobj_root; This should be a local variable. > +static struct kobj_type cma_ktype = { > + .release = cma_kobj_release, > + .sysfs_ops = &kobj_sysfs_ops, > + .default_groups = cma_groups I'd add a comma to the end, for consistency. > +};
24.03.2021 08:44, Minchan Kim пишет: > On Tue, Mar 23, 2021 at 09:47:27PM -0700, John Hubbard wrote: >> On 3/23/21 8:27 PM, Minchan Kim wrote: >> ... >>>>> +static int __init cma_sysfs_init(void) >>>>> +{ >>>>> + unsigned int i; >>>>> + >>>>> + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); >>>>> + if (!cma_kobj_root) >>>>> + return -ENOMEM; >>>>> + >>>>> + for (i = 0; i < cma_area_count; i++) { >>>>> + int err; >>>>> + struct cma *cma; >>>>> + struct cma_kobject *cma_kobj; >>>>> + >>>>> + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); >>>>> + if (!cma_kobj) { >>>>> + kobject_put(cma_kobj_root); >>>>> + return -ENOMEM; >>>> >>>> This leaks little cma_kobj's all over the floor. :) >>> >>> I thought kobject_put(cma_kobj_root) should deal with it. No? >>> >> If this fails when i > 0, there will be cma_kobj instances that >> were stashed in the cma_areas[] array. But this code only deletes >> the most recently allocated cma_kobj, not anything allocated on >> previous iterations of the loop. > > Oh, I misunderstood that destroying of root kobject will release > children recursively. Seems not true. Go back to old version. > > > index 16c81c9cb9b7..418951a3f138 100644 > --- a/mm/cma_sysfs.c > +++ b/mm/cma_sysfs.c > @@ -80,20 +80,19 @@ static struct kobj_type cma_ktype = { > static int __init cma_sysfs_init(void) > { > unsigned int i; > + int err; > + struct cma *cma; > + struct cma_kobject *cma_kobj; > > cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > if (!cma_kobj_root) > return -ENOMEM; > > for (i = 0; i < cma_area_count; i++) { > - int err; > - struct cma *cma; > - struct cma_kobject *cma_kobj; > - > cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > if (!cma_kobj) { > - kobject_put(cma_kobj_root); > - return -ENOMEM; > + err = -ENOMEM; > + goto out; > } > > cma = &cma_areas[i]; > @@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void) > cma_kobj_root, "%s", cma->name); > if (err) { > kobject_put(&cma_kobj->kobj); > - kobject_put(cma_kobj_root); > - return err; > + goto out; > } > } > > return 0; > +out: > + while (--i >= 0) { > + cma = &cma_areas[i]; > + > + kobject_put(&cma->kobj->kobj); > + kfree(cma->kobj); > + cma->kobj = NULL; > + } > + kobject_put(cma_kobj_root); > + > + return err; > } > subsys_initcall(cma_sysfs_init); Since we don't care about the order in which kobjects are put, I'd write it in this way, which I think looks cleaner: static void cma_sysfs_cleanup(struct kobject *cma_kobj_root) { struct cma *cma = cma_areas; unsigned int i; for (i = 0; i < cma_area_count; i++, cma++) { if (!cma->kobj) break; kobject_put(&cma->kobj->kobj); } kobject_put(cma_kobj_root); } static int __init cma_sysfs_init(void) { struct kobject *cma_kobj_root; unsigned int i; cma_kobj_root = kobject_create_and_add("cma", mm_kobj); if (!cma_kobj_root) return -ENOMEM; for (i = 0; i < cma_area_count; i++) { struct cma_kobject *cma_kobj; struct cma *cma; int err; cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); if (!cma_kobj) { cma_sysfs_cleanup(cma_kobj_root); return -ENOMEM; } cma = &cma_areas[i]; cma->kobj = cma_kobj; cma_kobj->cma = cma; err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype, cma_kobj_root, "%s", cma->name); if (err) { cma_sysfs_cleanup(cma_kobj_root); return err; } } return 0; } subsys_initcall(cma_sysfs_init);
On Wed, Mar 24, 2021 at 03:33:07PM +0300, Dmitry Osipenko wrote: > 24.03.2021 04:05, Minchan Kim пишет: > > +static struct kobject *cma_kobj_root; > > This should be a local variable. Sure. > > > +static struct kobj_type cma_ktype = { > > + .release = cma_kobj_release, > > + .sysfs_ops = &kobj_sysfs_ops, > > + .default_groups = cma_groups > > I'd add a comma to the end, for consistency. Yub.
On Wed, Mar 24, 2021 at 03:37:02PM +0300, Dmitry Osipenko wrote: > 24.03.2021 08:44, Minchan Kim пишет: > > On Tue, Mar 23, 2021 at 09:47:27PM -0700, John Hubbard wrote: > >> On 3/23/21 8:27 PM, Minchan Kim wrote: > >> ... > >>>>> +static int __init cma_sysfs_init(void) > >>>>> +{ > >>>>> + unsigned int i; > >>>>> + > >>>>> + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > >>>>> + if (!cma_kobj_root) > >>>>> + return -ENOMEM; > >>>>> + > >>>>> + for (i = 0; i < cma_area_count; i++) { > >>>>> + int err; > >>>>> + struct cma *cma; > >>>>> + struct cma_kobject *cma_kobj; > >>>>> + > >>>>> + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > >>>>> + if (!cma_kobj) { > >>>>> + kobject_put(cma_kobj_root); > >>>>> + return -ENOMEM; > >>>> > >>>> This leaks little cma_kobj's all over the floor. :) > >>> > >>> I thought kobject_put(cma_kobj_root) should deal with it. No? > >>> > >> If this fails when i > 0, there will be cma_kobj instances that > >> were stashed in the cma_areas[] array. But this code only deletes > >> the most recently allocated cma_kobj, not anything allocated on > >> previous iterations of the loop. > > > > Oh, I misunderstood that destroying of root kobject will release > > children recursively. Seems not true. Go back to old version. > > > > > > index 16c81c9cb9b7..418951a3f138 100644 > > --- a/mm/cma_sysfs.c > > +++ b/mm/cma_sysfs.c > > @@ -80,20 +80,19 @@ static struct kobj_type cma_ktype = { > > static int __init cma_sysfs_init(void) > > { > > unsigned int i; > > + int err; > > + struct cma *cma; > > + struct cma_kobject *cma_kobj; > > > > cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > > if (!cma_kobj_root) > > return -ENOMEM; > > > > for (i = 0; i < cma_area_count; i++) { > > - int err; > > - struct cma *cma; > > - struct cma_kobject *cma_kobj; > > - > > cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > > if (!cma_kobj) { > > - kobject_put(cma_kobj_root); > > - return -ENOMEM; > > + err = -ENOMEM; > > + goto out; > > } > > > > cma = &cma_areas[i]; > > @@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void) > > cma_kobj_root, "%s", cma->name); > > if (err) { > > kobject_put(&cma_kobj->kobj); > > - kobject_put(cma_kobj_root); > > - return err; > > + goto out; > > } > > } > > > > return 0; > > +out: > > + while (--i >= 0) { > > + cma = &cma_areas[i]; > > + > > + kobject_put(&cma->kobj->kobj); > > + kfree(cma->kobj); > > + cma->kobj = NULL; > > + } > > + kobject_put(cma_kobj_root); > > + > > + return err; > > } > > subsys_initcall(cma_sysfs_init); > > Since we don't care about the order in which kobjects are put, I'd write it in this way, which I think looks cleaner: > Hmm, preference matter. That kinds of goto error handling for unwinding is familiar in kernel code and simple enough for me. I don't think readbility is bad enough to need another cleanup function at this moment. > static void cma_sysfs_cleanup(struct kobject *cma_kobj_root) > { > struct cma *cma = cma_areas; > unsigned int i; > > for (i = 0; i < cma_area_count; i++, cma++) { > if (!cma->kobj) > break; > > kobject_put(&cma->kobj->kobj); > } > > kobject_put(cma_kobj_root); > } > > static int __init cma_sysfs_init(void) > { > struct kobject *cma_kobj_root; > unsigned int i; > > cma_kobj_root = kobject_create_and_add("cma", mm_kobj); > if (!cma_kobj_root) > return -ENOMEM; > > for (i = 0; i < cma_area_count; i++) { > struct cma_kobject *cma_kobj; > struct cma *cma; > int err; > > cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); > if (!cma_kobj) { > cma_sysfs_cleanup(cma_kobj_root); > return -ENOMEM; > } > > cma = &cma_areas[i]; > cma->kobj = cma_kobj; > cma_kobj->cma = cma; > err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype, > cma_kobj_root, "%s", cma->name); > if (err) { > cma_sysfs_cleanup(cma_kobj_root); > return err; > } > } > > return 0; > } > subsys_initcall(cma_sysfs_init);
diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-cma b/Documentation/ABI/testing/sysfs-kernel-mm-cma new file mode 100644 index 000000000000..02b2bb60c296 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-mm-cma @@ -0,0 +1,25 @@ +What: /sys/kernel/mm/cma/ +Date: Feb 2021 +Contact: Minchan Kim <minchan@kernel.org> +Description: + /sys/kernel/mm/cma/ contains a subdirectory for each CMA + heap name (also sometimes called CMA areas). + + Each CMA heap subdirectory (that is, each + /sys/kernel/mm/cma/<cma-heap-name> directory) contains the + following items: + + alloc_pages_success + alloc_pages_fail + +What: /sys/kernel/mm/cma/<cma-heap-name>/alloc_pages_success +Date: Feb 2021 +Contact: Minchan Kim <minchan@kernel.org> +Description: + the number of pages CMA API succeeded to allocate + +What: /sys/kernel/mm/cma/<cma-heap-name>/alloc_pages_fail +Date: Feb 2021 +Contact: Minchan Kim <minchan@kernel.org> +Description: + the number of pages CMA API failed to allocate diff --git a/mm/Kconfig b/mm/Kconfig index 24c045b24b95..febb7e8e24de 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -513,6 +513,13 @@ config CMA_DEBUGFS help Turns on the DebugFS interface for CMA. +config CMA_SYSFS + bool "CMA information through sysfs interface" + depends on CMA && SYSFS + help + This option exposes some sysfs attributes to get information + from CMA. + config CMA_AREAS int "Maximum count of the CMA areas" depends on CMA diff --git a/mm/Makefile b/mm/Makefile index 72227b24a616..56968b23ed7a 100644 --- a/mm/Makefile +++ b/mm/Makefile @@ -109,6 +109,7 @@ obj-$(CONFIG_CMA) += cma.o obj-$(CONFIG_MEMORY_BALLOON) += balloon_compaction.o obj-$(CONFIG_PAGE_EXTENSION) += page_ext.o obj-$(CONFIG_CMA_DEBUGFS) += cma_debug.o +obj-$(CONFIG_CMA_SYSFS) += cma_sysfs.o obj-$(CONFIG_USERFAULTFD) += userfaultfd.o obj-$(CONFIG_IDLE_PAGE_TRACKING) += page_idle.o obj-$(CONFIG_DEBUG_PAGE_REF) += debug_page_ref.o diff --git a/mm/cma.c b/mm/cma.c index 908f04775686..204e349a0c25 100644 --- a/mm/cma.c +++ b/mm/cma.c @@ -507,10 +507,13 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align, pr_debug("%s(): returned %p\n", __func__, page); out: - if (page) + if (page) { count_vm_event(CMA_ALLOC_SUCCESS); - else + cma_sysfs_account_success_pages(cma, count); + } else { count_vm_event(CMA_ALLOC_FAIL); + cma_sysfs_account_fail_pages(cma, count); + } return page; } diff --git a/mm/cma.h b/mm/cma.h index 42ae082cb067..2298a2e736a6 100644 --- a/mm/cma.h +++ b/mm/cma.h @@ -3,6 +3,12 @@ #define __MM_CMA_H__ #include <linux/debugfs.h> +#include <linux/kobject.h> + +struct cma_kobject { + struct cma *cma; + struct kobject kobj; +}; struct cma { unsigned long base_pfn; @@ -16,6 +22,14 @@ struct cma { struct debugfs_u32_array dfs_bitmap; #endif char name[CMA_MAX_NAME]; +#ifdef CONFIG_CMA_SYSFS + /* the number of CMA page successful allocations */ + atomic64_t nr_pages_succeeded; + /* the number of CMA page allocation failures */ + atomic64_t nr_pages_failed; + /* kobject requires dynamic object */ + struct cma_kobject *kobj; +#endif }; extern struct cma cma_areas[MAX_CMA_AREAS]; @@ -26,4 +40,13 @@ static inline unsigned long cma_bitmap_maxno(struct cma *cma) return cma->count >> cma->order_per_bit; } +#ifdef CONFIG_CMA_SYSFS +void cma_sysfs_account_success_pages(struct cma *cma, size_t count); +void cma_sysfs_account_fail_pages(struct cma *cma, size_t count); +#else +static inline void cma_sysfs_account_success_pages(struct cma *cma, + size_t count) {}; +static inline void cma_sysfs_account_fail_pages(struct cma *cma, + size_t count) {}; +#endif #endif diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c new file mode 100644 index 000000000000..c3791a032dc5 --- /dev/null +++ b/mm/cma_sysfs.c @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * CMA SysFS Interface + * + * Copyright (c) 2021 Minchan Kim <minchan@kernel.org> + */ + +#include <linux/cma.h> +#include <linux/kernel.h> +#include <linux/slab.h> + +#include "cma.h" + +void cma_sysfs_account_success_pages(struct cma *cma, size_t count) +{ + atomic64_add(count, &cma->nr_pages_succeeded); +} + +void cma_sysfs_account_fail_pages(struct cma *cma, size_t count) +{ + atomic64_add(count, &cma->nr_pages_failed); +} + +#define CMA_ATTR_RO(_name) \ + static struct kobj_attribute _name##_attr = __ATTR_RO(_name) + +#define to_cma_kobject(x) container_of(x, struct cma_kobject, kobj) + +static ssize_t alloc_pages_success_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct cma_kobject *cma_kobj = to_cma_kobject(kobj); + struct cma *cma = cma_kobj->cma; + + return sysfs_emit(buf, "%llu\n", + atomic64_read(&cma->nr_pages_succeeded)); +} +CMA_ATTR_RO(alloc_pages_success); + +static ssize_t alloc_pages_fail_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct cma_kobject *cma_kobj = to_cma_kobject(kobj); + struct cma *cma = cma_kobj->cma; + + return sysfs_emit(buf, "%llu\n", atomic64_read(&cma->nr_pages_failed)); +} +CMA_ATTR_RO(alloc_pages_fail); + +static void cma_kobj_release(struct kobject *kobj) +{ + struct cma_kobject *cma_kobj = to_cma_kobject(kobj); + struct cma *cma = cma_kobj->cma; + + kfree(cma_kobj); + cma->kobj = NULL; +} + +static struct attribute *cma_attrs[] = { + &alloc_pages_success_attr.attr, + &alloc_pages_fail_attr.attr, + NULL, +}; +ATTRIBUTE_GROUPS(cma); + +static struct kobject *cma_kobj_root; + +static struct kobj_type cma_ktype = { + .release = cma_kobj_release, + .sysfs_ops = &kobj_sysfs_ops, + .default_groups = cma_groups +}; + +static int __init cma_sysfs_init(void) +{ + unsigned int i; + + cma_kobj_root = kobject_create_and_add("cma", mm_kobj); + if (!cma_kobj_root) + return -ENOMEM; + + for (i = 0; i < cma_area_count; i++) { + int err; + struct cma *cma; + struct cma_kobject *cma_kobj; + + cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL); + if (!cma_kobj) { + kobject_put(cma_kobj_root); + return -ENOMEM; + } + + cma = &cma_areas[i]; + cma->kobj = cma_kobj; + cma_kobj->cma = cma; + err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype, + cma_kobj_root, "%s", cma->name); + if (err) { + kobject_put(&cma_kobj->kobj); + kobject_put(cma_kobj_root); + return err; + } + } + + return 0; +} +subsys_initcall(cma_sysfs_init);