On 10/7/20 6:40 PM, Cornelia Huck wrote:
> On Wed, 7 Oct 2020 16:33:37 +0200
> Jan Höppner <hoeppner@xxxxxxxxxxxxx> wrote:
>
>>>>>> +static inline void dasd_path_release(struct kobject *kobj)
>>>>>> +{
>>>>>> +/* Memory for the dasd_path kobject is freed when dasd_free_device() is called */
>>>>>> +}
>>>>>> +
>>>>>
>>>>> As already said, I don't think that's a correct way to implement this.
>>>>>
>>>>
>>>> As you correctly pointed out, our release function doesn't do anything.
>>>> This is because our path data is a (static) part of our device.
>>>> This data is critical to keep our devices operational.
>>>> We can't simply rely on allocated memory if systems are under stress.
>>>
>>> Yes, avoiding freeing and reallocating memory certainly makes sense.
>>>
>>>>
>>>> Having this data dynamically allocated involves a lot of rework of our
>>>> path handling as well. There are a few things that are subject to improvement
>>>> and evaluating whether our dasd_path structures can be dynamic is one of
>>>> these things. However, even then, the above concern persists and I
>>>> highly doubt that dynamic dasd_paths objects are doable for us at this
>>>> moment.
>>>>
>>>> I do understand the concerns, however, we release the memory for dasd_path
>>>> structures eventually when dasd_free_device() is called. Until that point,
>>>> the data has to be kept alive. The rest is taking care of by the kobject
>>>> library.
>>>
>>> Yes, there doesn't seem to be any memory leakage.
>>>
>>>> In our path handling we also make sure that we can always verify/validate
>>>> paths information even if a system is under high memory pressure. Another
>>>> reason why it would contradictory for dasd_path objects to be dynamic.
>>>>
>>>> I hope this explains the reasoning behind the release function.
>>>
>>> I understand where you're coming from.
>>>
>>> However, "static" kobjects (in the sense of "we may re-register the
>>> same kobject") are still problematic. Is there any way to simply
>>> "disappear" path objects that are not valid at the moment, or mark them
>>> as not valid?
>>
>> You could use kobject_del(), but it is rather intended to be used for
>> a two-stage removal of the kobject.
>>
>>>
>>> Also, the simple act of registering/unregistering a kobject already
>>> creates stress from its sysfs interactions... it seems you should try
>>> to avoid that as well?
>>>
>>
>> We don't re-register kobjects over and over again. The kobjects are
>> infact initialized and created only _once_. This is done either during
>> device initialization (after dasd_eckd_read_conf() in
>> dasd_eckd_check_characteristics()) or when a path is newly added
>> (in the path event handler).
>> The kobject will stay until the memory for the whole device is being
>> freed. This is also the reason why the kobject can stay initialized and
>> we track ourselves whether we did the initialization/creation already
>> (which we check e.g. when a path is removed and added again).
>> So, instead of the release function freeing the kobject data,
>> it is done by our dasd_free_device() (same thing, different function IMHO).
>>
>> I think the concerns would be more worrisome if we'd remove/add
>> the kobjects every time. And then I agree, we'd run into trouble.
>>
>
> The thing that tripped me is
>
> +void dasd_path_remove_kobj(struct dasd_device *device, int chp)
> +{
> + if (device->path[chp].in_sysfs) {
> + kobject_put(&device->path[chp].kobj);
> + device->path[chp].in_sysfs = false;
> + }
> +}
>
> As an exported function, it is not clear where this may be called from.
> Given your explanation above (and some more code reading on my side),
> the code looks ok in its current incarnation (but non-idiomatic).
>
> Is there a way to check that indeed nobody re-adds a previously removed
> path object due to a (future) programming error? And maybe add a
> comment that you must never re-register a path? "The path is gone,
> let's remove the object" looks quite tempting.
>
A comment is the minimum I can think of at the moment and
I'll prepare a fixup patch or a new version of this patch that adds
a proper comment for this function.
Other ways to protect the usage must be investigated.
I have to discuss with Stefan what the best approach would be as the patchset
is supposed to be ready for upstream integration.
I'd prefer a fixup patch that we could send with at least one more fixup patch
that we have in the pipe already. Let's see. I hope that's fine with you
(and Jens obviously) so far.
regards,
Jan
