Re: [Xen-devel] [PATCH v1 02/10] libxl_internal: functions to lock / unlock domain configuration

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jul 17, 2014 at 12:29:11PM +0100, Ian Campbell wrote:
> On Wed, 2014-07-16 at 17:44 +0100, Wei Liu wrote:
> > On Wed, Jul 16, 2014 at 05:15:41PM +0100, Ian Campbell wrote:
> > > On Thu, 2014-07-10 at 15:32 +0100, Wei Liu wrote:
> > > > Simple file lock taken from xl to serialise access to "libxl-json" file.
> > > 
> > > Do you mean libxl here?
> > > 
> > 
> > No. I did mean "xl". This implementation is "borrowed" from "xl".
> 
> 
> Ah, I thought you meant the lock was taken (as in locked) from xl, which
> made no sense. Suggest inserting the word "implementation" in that
> sentence somewhere.
> 

No problem.

> > > > If a thread cannot get hold of the lock it waits due to F_SETLKW.
> > > 
> > > How is deadlock avoided here?
> > > 
> > 
> > This patch only provides "primitive" to lock and unlock. Avoiding
> > deadlock is out of scope of this patch. That pretty much depends on the
> > users of this lock.
> 
> Hrm. It would be conventional to describe for the benefit of those users
> what the requirements are. e.g. by defining a locking hierarchy for
> libxl. e.g. relative to the CTX lock.
> 

This lock is only used internally by libxl. I guess I should document it
in libxl_internal.h? There doesn't seem to a existing doc on locking
hierarchy in either libxl.h or libxl_internal.h. I guess the
introduction of this lock is the first instance that needs documented?

> It might also be necessary  to specify what the various event callbacks
> are allowed to assume here -- but Ian knows the event code best so I'll
> let him comment.
> 
> > > > In order to generate lock file name, rename userdata_path to
> > > > libxl__userdata_path and declare it in libxl_internal.h
> > > 
> > > Given that you don't appear to be locking the userdata file itself that
> > > seems like an odd place for a lock to me, why not XEN_LOCK_DIR?
> > > 
> > 
> > So that the lock file itself gets deleted by libxl automatically. Using
> > XEN_LOCK_DIR requires some extra care to delete the lock file. And if
> > libxl user crashes for any reason, those files become stale. Leaving
> > them in one location is easier to clean them up.
> 
> There's only one (empty) file though, isn't there? and it will be reused
> by any subsequent libxl call from any process. IOW I don't think you
> need to concern yourself with removing it.
> 

No, it's per-domain, so that we don't content on it (now I see why you
said it would be a bottleneck because you thought there's only one
file). Leaving a bunch of stale files is not that nice, isn't it?

> > > Did you consider locking the actual file though? That would avoid this
> > > lock being a bottleneck...
> > > 
> > 
> > It's just that part of this series was taken from my older series where
> > some macro already had its own code to manipulate configuration file,
> > plumbing locking code (passing fd around) and those macros together
> > would make code look complete new. I was a bit worried I would introduce
> > too many changes that might confuse you maintainers. I can lock the file
> > itself if you prefer. 
> > 
> > But I don't quite follow how this lock can be a bottleneck. Locking the
> > actual file is different from locking a lock file? Isn't there one file
> > to be locked anyway?
> 
> If you lock the actual file then you are only serialising operations on
> a single domain, operations on other domains can go ahead (each with
> their own lock).
> 
> If you lock a single file then all operations on all domains are
> serialised.
> 

See above.

Wei.

> Ian.
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel