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;
+}
[PATCH v6] mm: cma: support sysfs
Since CMA is getting used more widely, it's more important to
keep monitoring CMA statistics for system health since it's
directly related to user experience.
This patch introduces sysfs statistics for CMA, in order to provide
some basic monitoring of the CMA allocator.
* the number of CMA page successful allocations
* the number of CMA page allocation failures
These two values allow the user to calculate the allocation
failure rate for each CMA area.
e.g.)
/sys/kernel/mm/cma/WIFI/alloc_pages_[success|fail]
/sys/kernel/mm/cma/SENSOR/alloc_pages_[success|fail]
/sys/kernel/mm/cma/BLUETOOTH/alloc_pages_[success|fail]
The cma_stat was intentionally allocated by dynamic allocation
to harmonize with kobject lifetime management.
https://lore.kernel.org/linux-mm/ycoamxqt6dzkc...@kroah.com/
Reviewed-by: Greg Kroah-Hartman
Reviewed-by: John Hubbard
Tested-by: Dmitry Osipenko
Signed-off-by: Minchan Kim
---
>From v5 -
>https://lore.kernel.org/linux-mm/20210323195050.2577017-1-minc...@kernel.org/
* refactoring and fix typo - digetx@
>From v4 -
>https://lore.kernel.org/linux-mm/20210309062333.3216138-1-minc...@kernel.org/
* fix corruption - digetx@
>From v3 -
>https://lore.kernel.org/linux-mm/20210303205053.2906924-1-minc...@kernel.org/
* fix ZERO_OR_NULL_PTR - kernel test robot
* remove prefix cma - david@
* resolve conflict with vmstat cma in mmotm - akpm@
* rename stat name with success|fail
>From v2 -
>https://lore.kernel.org/linux-mm/20210208180142.2765456-1-minc...@kernel.org/
* sysfs doc and description modification - jhubbard
>From v1 -
>https://lore.kernel.org/linux-mm/20210203155001.4121868-1-minc...@kernel.org/
* fix sysfs build and refactoring - willy
Documentation/ABI/testing/sysfs-kernel-mm-cma | 25
mm/Kconfig| 7 ++
mm/Makefile | 1 +
mm/cma.c | 7 +-
mm/cma.h | 23
mm/cma_sysfs.c| 107 ++
6 files changed, 168 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-cma
create mode 100644 mm/cma_sysfs.c
diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-cma
b/Documentation/ABI/testing/sysfs-kernel-mm-cma
new file mode 100644
index ..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
+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/ directory) contains the
+ following items:
+
+ alloc_pages_success
+ alloc_pages_fail
+
+What: /sys/kernel/mm/cma//alloc_pages_success
+Date: Feb 2021
+Contact: Minchan Kim
+Description:
+ the number of pages CMA API succeeded to allocate
+
+What: /sys/kernel/mm/cma//alloc_pages_fail
+Date: Feb 2021
+Contact: Minchan Kim
+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
