xen-dom-mgmt: Fix potential concurrency issues #72
Conversation
Deedone
force-pushed
the
domain_refcount
branch
from
ba9d728 to
f44bce5
Compare
|
Huh. lock on lock on lock + ref count. It looks very over-complicated and over-designed. In my strong opinion, the locking is the problem of Zephyr app and not a library. More over, domain management/control process should be simple and straight forward in Zephyr app which definitely should not require to have ref_count at minimum. And I think It all should just work with one global lock. |
A am not sure I got this logic. |
Deedone
force-pushed
the
domain_refcount
branch
from
f44bce5 to
1555f05
Compare
|
Moved freeing to put_domain and removed the per-domain lock. |
Deedone
force-pushed
the
domain_refcount
branch
from
1555f05 to
eedc047
Compare
|
@Deedone Could you send first patch alone as it's definitely fix "xen_console: Fix deadlock in xen_stop_domain_console()". Regarding protection - My point stands, per-doms locks and ref_count is over complications and just domctrl_global_lock and global_console_lock should be enough. |
What I meant, is that whole domain_destroy should be triggered by put_domain(). Because current implementation has no sense: someone may be holding domain reference and trying to do something with it, while you are stopping console, destroying xenstore entries and so on. There is no sense in having reference for a domain, when it basically is destroyed already. At least I see no use case for this. Maybe I am wrong. I believe @GrygiriiS is right and we have no use for reference counting for now. Maybe global Anyways, @firscity is maintaining this project and he knows it better, so last word is his. In the meantime, you can have my:
|
Yeah, this part should be changed as you suggested before.
We already have at least 3 ways to create and manage domains - directly from application via |
There was a problem hiding this comment.
With checkpatch fixed:
Reviewed-by: Dmytro Firsov <[email protected]>
xen-dom-mgmt/src/xen-dom-mgmt.c
Outdated
| domain->refcount--; | ||
| if (domain->refcount == 0) { | ||
| if (domain->f_dom0less) { | ||
| LOG_ERR("dom0less domain#%u operation not supported", domain->domid); |
There was a problem hiding this comment.
I am not fully understand what will happen in this case. We will be stuck with struct xen_domain that has refcount==0 forever?
Also you need to unlock the mutex.
Shall we replace this with ASSERT(!domain->f_dom0less) ? @firscity, @GrygiriiS ?
Deedone
force-pushed
the
domain_refcount
branch
2 times, most recently
from
c59bc5f to
d3a1abc
Compare
|
Added assert, fixed checkpatch |
Make xs_remove_xenstore_backends accept a pointer to xen_domain instead of domain_id to improve ownership management of the domain object. Practically this removes the need to get an extra reference to the domain during the destroy sequence. Signed-off-by: Mykyta Poturai <[email protected]> Reviewed-by: Dmytro Firsov <[email protected]> Acked-by: Volodymyr Babchuk <[email protected]>
Add reference counting to the domain structure to make accessing it safe without global locks. domain_create initializes the refcount to 1 to prevent the domain from being freed right away, this original reference is dropped in domain_destroy to trigger freeing the domain. Also update test headers to keep them in sync with the changes. Signed-off-by: Mykyta Poturai <[email protected]> Reviewed-by: Dmytro Firsov <[email protected]> Acked-by: Volodymyr Babchuk <[email protected]>
Deedone
force-pushed
the
domain_refcount
branch
from
d3a1abc to
a43ff0e
Compare
Add refcounting to the domain structure to make it thread-safe. Fix possible deadlock in console deinit.