dan.j.williams@ wrote: > Dave Jiang wrote: [snip]
> > > The lock ordering can cause potential deadlock. There are instances > > where hmem_resource_lock is taken after (node_chain).rwsem, and vice > > versa. Narrow the scope of hmem_resource_lock in hmem_register_resource() > > to avoid the circular locking dependency. The locking is only needed when > > hmem_active needs to be protected. > > It is only strictly necessary for hmem_active, but it happened to be > protecting theoretical concurrent callers of hmat_register_resource(). Are you saying hmem_resource_lock was also protecting platform_initialized? > I > do not think it can happen in practice, but it is called by both initial > init and runtime notifier. The notifier path does: > > hmat_callback() -> hmat_register_target() > > That path seems impossible to add new hmem devices, but it is burning > cycles walking through all the memory ranges associated with a target > only to find that they are already registered. I think that can be > cleaned up with an unlocked check of target->registered. > > If that loses some theoretical race then your new > hmem_request_resource() will pick a race winner for that target. > > Otherwise, the code *looks* like it has a TOCTOU race with > platform_initialized. Yea, I had the same concern; but it's not WRT hmem_active. > So feels like some comments and cleanups to make > that clear are needed. > > Really I think hmat_callback() path should not be doing *any* > registration work, only update work. I think this makes sense to me but what about: hmat_init() -> hmat_register_targets() -> hmat_register_target() Does that still need to register the device? Ira [snip]
