Re: [patch] turning devctl into a "multiple openable" device

On Fri, Dec 02, 2011 at 12:17:21AM +0100, Baptiste Daroussin wrote:
> On Wed, Nov 30, 2011 at 05:20:17PM +0100, Olivier Houchard wrote:
> > On Wed, Nov 30, 2011 at 06:04:50PM +0200, Kostik Belousov wrote:
> > > > > I wonder why the waiting_threads stuff is needed at all. The cv could
> > > > > be woken up unconditionally everytime. What is the reason for the cv_wait
> > > > > call in cdevpriv data destructor ? You cannot have a thread doing e.g.
> > > > > read on the file descriptor while destructor is run.
> > > > > 
> > > > 
> > > > What will prevent you from having a thread stuck in read(), while an another 
> > > > one close() the fd ?
> > > > 
> > > Nothing, but file reference count goes to zero only after the thread
> > > stuck in read is unstuck. Cdevpriv destructor is run only when file
> > > reference count becomes zero, i.e. there can be no any accessing threads,
> > > and new accesses are impossible since file descriptors also own references
> > > on the file.
> > 
> > Right, I was a bit confused, this part can be removed.
> > 
> > Regards,
> > 
> > Olivier
> 
> Here is a new version of the patch mostly reworked by Olivier,
> It doesn't duplicate anymore the devq, and fix all that have been
> spotted here previously.
> 
> http://people.freebsd.org/~bapt/devctl_multi_open.diff
> 
> bonus, it removes the needless giant lock
I do not see a need in the use of refcount KPI, since it seems that
all manipulations of n->refcount happen under global_devctl.mtx.
Am I missed the place ?

Releasing refcount on dev_event_info before using it in devread()
allows other thread to free the memory while current thread still
uses it.

Stylish notes:
mtx member in struct global_devctl has weird indentation;
please move declaration of n2 into the declaration block of devdtor();
devread():should_free is initialized at the declaration site;
devctl_queue_data_f() has stray empty line ater the while { loop line,
while need blank line at the start of devctl_queue_data() is removed.