lkml.org 
[lkml]   [2021]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v6] mm: cma: support sysfs
    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);

    \
     
     \ /
      Last update: 2021-03-24 16:20    [W:2.449 / U:0.616 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site