Messages in this thread | | | Date | Fri, 22 Jun 2012 13:25:06 +0800 | Subject | Re: [PATCH 1/1] of: reform prom_update_property function | From | Dong Aisheng <> |
| |
On Fri, Jun 22, 2012 at 8:01 AM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Thu, 2012-06-21 at 17:37 +0800, Dong Aisheng wrote: >> Maybe we could change it as as follows. >> It looks then the code follow is the same as before. >> Do you think if it's ok? >> >> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c >> index 7b3bf76..4c237f4 100644 >> --- a/arch/powerpc/platforms/pseries/reconfig.c >> +++ b/arch/powerpc/platforms/pseries/reconfig.c >> @@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize) >> if (!next_prop) >> return -EINVAL; >> >> + if (!strlen(name) >> + return -ENODEV; >> + >> newprop = new_property(name, length, value, NULL); >> if (!newprop) >> return -ENOMEM; >> @@ -450,13 +453,6 @@ static int do_update_property(char *buf, size_t bufsize) >> if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size")) >> slb_set_size(*(int *)value); >> >> - oldprop = of_find_property(np, name,NULL); >> - if (!oldprop) { >> - if (strlen(name)) >> - return prom_add_property(np, newprop); >> - return -ENODEV; >> - } > > No: > > IE. Old code did: > > if (property doesn't exist) { > if (has a name) > create_it() > return -ENODEV; > } > What i saw is: if (property doesn't exist) { if (has a name) return create_it() return -ENODEV; } Which seems the same behavior as the new prop_update_property api. The only different is if no name, return -EINVAL; Am i wrong?
> What you propose is: > > if (!has_a_name) > return -ENODEV; > > Not at all the same semantic. > > .../... > >> > IE. The allocation of the "old" property isn't disposed of. It can't >> > because today we don't know whether any of those pointers was >> > dynamically allocated or not. IE they could point to the fdt > >> Hmm, i did not see static allocated property before. >> Where can we see an exist case? > > Almost all your property names and values. They are pointers to the > original fdt block, so you can't free them. But dynamically added > propreties will have kmalloc'ed pointers which should be freed. We need > to add flags to indicate that if we want to avoid leaking memory in very > dynamic environments. > Okay, got it, thanks for clarify.
>> If we really have this issue, it seems of_node_release also has the same issue, >> since it frees the property without distinguish whether the property is allocated >> dynamically. > > Well, actually we do have a flag: > > if (!of_node_check_flag(node, OF_DYNAMIC)) > return; > Oh, i see.
> So we use that. Good. So if update property uses that flag it should be > able to know when to free or not. I forgot we had that :-) > I'm still not sure whether we should free the property in update property function. Looking at the code, it seems prom_update_property actually does not remove it. It only move the property to "dead properties" list. See the function comment in kernel: /** * prom_remove_property - Remove a property from a node. * * Note that we don't actually remove it, since we have given out * who-knows-how-many pointers to the data using get-property. * Instead we just move the property to the "dead properties" * list, so it won't be found any more. */
Finally the dead properties will be freed in of_release_node if the node has no users after calling of_node_put.
static void of_node_release(struct kref *kref) { ..... struct property *prop = node->properties; ....... if (!of_node_check_flag(node, OF_DYNAMIC)) return;
while (prop) { struct property *next = prop->next; kfree(prop->name); kfree(prop->value); kfree(prop); prop = next;
if (!prop) { prop = node->deadprops; node->deadprops = NULL; } } kfree(node->full_name); kfree(node->data); kfree(node); }
So it looks to me there's no memory leak, did i understand wrong?
>> > string list, data block, or could be bootmem ... or could be >> > actual kernel memory. >> > >> > We might want to extend the struct property to contain indications of >> > the allocation type so we can kfree dynamic properties properly. >> > >> I wonder the simplest way may be not allow static allocated property, like dt >> node does i guess. > > No way. We generate the device-tree way before we have an allocator > available. > Oh, got it.
>> > But then there's the question of the lifetime of a property... since >> > they aren't reference counted like nodes are. >> > >> Yes, that's a real exist problem. >> >> Anyway, i guess we could do that work of this problem in another patch >> rather than have to in this patch series. > > Cheers, > Ben. > >
Regards Dong Aisheng -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |