Re: [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 = _areas[i];
> > @@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void)
> >cma_kobj_root, "%s", cma->name);
> > if (err) {
> > kobject_put(_kobj->kobj);
> > - kobject_put(cma_kobj_root);
> > - return err;
> > + goto out;
> > }
> > }
> >
> > return 0;
> > +out:
> > + while (--i >= 0) {
> > + cma = _areas[i];
> > +
> > + kobject_put(>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(>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 = _areas[i];
> cma->kobj = cma_kobj;
> cma_kobj->cma = cma;
> err = kobject_init_and_add(_kobj->kobj, _ktype,
> cma_kobj_root, "%s", cma->name);
> if (err) {
> cma_sysfs_cleanup(cma_kobj_root);
> return err;
> }
> }
>
> return 0;
> }
> subsys_initcall(cma_sysfs_init);
Re: [PATCH v6] mm: cma: support sysfs
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 = _sysfs_ops,
> > + .default_groups = cma_groups
>
> I'd add a comma to the end, for consistency.
Yub.
Re: [PATCH v6] mm: cma: support sysfs
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 = _areas[i];
> @@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void)
>cma_kobj_root, "%s", cma->name);
> if (err) {
> kobject_put(_kobj->kobj);
> - kobject_put(cma_kobj_root);
> - return err;
> + goto out;
> }
> }
>
> return 0;
> +out:
> + while (--i >= 0) {
> + cma = _areas[i];
> +
> + kobject_put(>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(>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 = _areas[i];
cma->kobj = cma_kobj;
cma_kobj->cma = cma;
err = kobject_init_and_add(_kobj->kobj, _ktype,
cma_kobj_root, "%s", cma->name);
if (err) {
cma_sysfs_cleanup(cma_kobj_root);
return err;
}
}
return 0;
}
subsys_initcall(cma_sysfs_init);
Re: [PATCH v6] mm: cma: support sysfs
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 = _sysfs_ops,
> + .default_groups = cma_groups
I'd add a comma to the end, for consistency.
> +};
Re: [PATCH v6] mm: cma: support sysfs
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,
--
John Hubbard
NVIDIA
Re: [PATCH v6] mm: cma: support sysfs
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 = _areas[i];
> > @@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void)
> > cma_kobj_root, "%s", cma->name);
> > if (err) {
> > kobject_put(_kobj->kobj);
> > - kobject_put(cma_kobj_root);
> > - return err;
> > + goto out;
> > }
> > }
> >
> > return 0;
> > +out:
> > + while (--i >= 0) {
> > + cma = _areas[i];
> > +
> > + kobject_put(>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.
Re: [PATCH v6] mm: cma: support sysfs
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 = _areas[i];
@@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void)
cma_kobj_root, "%s", cma->name);
if (err) {
kobject_put(_kobj->kobj);
- kobject_put(cma_kobj_root);
- return err;
+ goto out;
}
}
return 0;
+out:
+ while (--i >= 0) {
+ cma = _areas[i];
+
+ kobject_put(>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
Re: [PATCH v6] mm: cma: support sysfs
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 = _areas[i];
@@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void)
cma_kobj_root, "%s", cma->name);
if (err) {
kobject_put(_kobj->kobj);
- kobject_put(cma_kobj_root);
- return err;
+ goto out;
}
}
return 0;
+out:
+ while (--i >= 0) {
+ cma = _areas[i];
+
+ kobject_put(>kobj->kobj);
+ kfree(cma->kobj);
+ cma->kobj = NULL;
+ }
+ kobject_put(cma_kobj_root);
+
+ return err;
}
subsys_initcall(cma_sysfs_init);
Re: [PATCH v6] mm: cma: support sysfs
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,
--
John Hubbard
NVIDIA
Re: [PATCH v6] mm: cma: support sysfs
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 ..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
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include "cma.h"
> > +
> > +void cma_sysfs_account_success_pages(struct cma *cma, size_t count)
> > +{
> > + atomic64_add(count, >nr_pages_succeeded);
> > +}
> > +
> > +void cma_sysfs_account_fail_pages(struct cma *cma, size_t count)
> > +{
> > + atomic64_add(count, >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(>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(>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(>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(>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[] = {
> > + _pages_success_attr.attr,
> > + _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 = _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 = _areas[i];
> > + cma->kobj = cma_kobj;
> > +
Re: [PATCH v6] mm: cma: support sysfs
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 ..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
+ */
+
+#include
+#include
+#include
+
+#include "cma.h"
+
+void cma_sysfs_account_success_pages(struct cma *cma, size_t count)
+{
+ atomic64_add(count, >nr_pages_succeeded);
+}
+
+void cma_sysfs_account_fail_pages(struct cma *cma, size_t count)
+{
+ atomic64_add(count, >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(>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(>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(>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(>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[] = {
+ _pages_success_attr.attr,
+ _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 = _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 = _areas[i];
+ cma->kobj = cma_kobj;
+ cma_kobj->cma = cma;
+ err = kobject_init_and_add(_kobj->kobj, _ktype,
+ cma_kobj_root, "%s", cma->name);
+ if (err) {
+ kobject_put(_kobj->kobj);
+ kobject_put(cma_kobj_root);
+ return err;
Hopefully this little bit of logic could also go into the cleanup
routine.
+ }
+ }
+
+ return 0;
+}
