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,
--
John Hubbard
NVIDIA
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.