[openib-general] Re: [PATCH] core+mthca: questions and proposal: kill ib_(de)alloc_device

[Roland Dreier]

>>>>> "Michael" == Michael S Tsirkin <mst at mellanox.co.il> writes:

    Michael> Guys, looking with Leonid Keller at device.c raised
    Michael> several questions with regard to what
    Michael> ib_alloc_device/ib_dealloc_device do:

I agree this code is probably wrong and needs to be fixed up, but I
don't think it's quite as simple as your patch unfortunately.  See below.

    Michael> 1. Why is ib_device_register_sysfs called from
    Michael> ib_register_device, but ib_device_unregister_sysfs from
    Michael> ib_dealloc_device?

It probably isn't the best design, but ib_device_unregister_sysfs() is
what triggers the final free of the device structure.

    Michael> 2. Who is supposed to set reg_state back to
    Michael> IB_DEV_UNINITIALIZED?  Without it ib_dealloc_device does
    Michael> not seem to free the device structure. Is this a memory
    Michael> leak?

No, it's not a leak.  ib_dealloc_device() doesn't actually perform the
freeing unless the device has never been registered at all.  If it has
been registered and then unregistered, ib_device_unregister_sysfs()
unregisters the class_device, which ends up calling
ib_device_release() when all references are gone.

    Michael> 3. ib_alloc_device does not set reg_state, it seems to
    Michael> rely on the fact that IB_DEV_UNINITIALIZED = 0. Is that
    Michael> intentional?

No, that should be made explicit.

    Michael> 4. For ib_alloc_device/ib_dealloc_device to work
    Michael> properly, it seems that the device structure must have
    Michael> ib_device as the first member.  Is this limitation
    Michael> documented anywhere?

Hmm, not explicitly that I can see.

    Michael> 5. Why do we need reg_state in the device, at all?  I
    Michael> thought we can trust providers to call
    Michael> register/unregister in proper order?

The only thing reg_state is really used for is to tell if a device has
ever been registered.  If it hasn't, then there's no sysfs stuff, so
ib_dealloc_device() can just free it.  This could be reworked so that
struct ib_device has an embedded kref, and each sysfs registration
just takes a reference on the ib_device.

    Michael> What do you say we simply let providers allocate the
    Michael> structure?  Can you really go wrong reducing the line
    Michael> count by more than 50 :) ?

Unfortunately I think so.  The current code isn't safe and has a bunch
of holes, but the goal is correct: keep some context around until the
last sysfs reference has been released.

 - R.