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.
pgp5tdbw3yh7H.pgp
Description: PGP signature
Re: [patch] turning devctl into a multiple openable device
On Wednesday, November 30, 2011 11:41:25 am Hans Petter Selasky wrote: On Wednesday 30 November 2011 13:43:20 Baptiste Daroussin wrote: Hi all, With the help of cognet, I wrote a patch to turn devctl into a multiple openable device, that mean that it will allow to open /dev/devctl in multiple programs, for example hald and everythings that want to receive notification from the device won't need to depend on haveing devd running. here is the patch: http://people.freebsd.org/~bapt/devctl_multi_open.diff regards, bapt Comment: Is the track-close flag set for the devfs sw? Not, it uses the far-superior devfs_set_cdevpriv(). -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [patch] turning devctl into a multiple openable device
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 regards, Bapt pgpcRpG0lN1YH.pgp Description: PGP signature
Re: [patch] turning devctl into a multiple openable device
On Wednesday, November 30, 2011 7:43:20 am Baptiste Daroussin wrote: Hi all, With the help of cognet, I wrote a patch to turn devctl into a multiple openable device, that mean that it will allow to open /dev/devctl in multiple programs, for example hald and everythings that want to receive notification from the device won't need to depend on haveing devd running. here is the patch: http://people.freebsd.org/~bapt/devctl_multi_open.diff Shouldn't devctl_queue_data_f() use the requested malloc() flags instead of hardcoding M_NOWAIT? Also, I know that it was an intentional design decisison by Warner to have the multiplexing of devctl data done in userland via devd rather than in the kernel. (I think he envisioned devd providing a UNIX domain socket or some such for other daemons to use to listen to events.) Have you asked him about this change? -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [patch] turning devctl into a multiple openable device
On Wed, Nov 30, 2011 at 10:05:11AM -0500, John Baldwin wrote: On Wednesday, November 30, 2011 7:43:20 am Baptiste Daroussin wrote: Hi all, With the help of cognet, I wrote a patch to turn devctl into a multiple openable device, that mean that it will allow to open /dev/devctl in multiple programs, for example hald and everythings that want to receive notification from the device won't need to depend on haveing devd running. here is the patch: http://people.freebsd.org/~bapt/devctl_multi_open.diff Shouldn't devctl_queue_data_f() use the requested malloc() flags instead of hardcoding M_NOWAIT? you are right, I'll fix that. Also, I know that it was an intentional design decisison by Warner to have the multiplexing of devctl data done in userland via devd rather than in the kernel. (I think he envisioned devd providing a UNIX domain socket or some such for other daemons to use to listen to events.) Have you asked him about this change? I haven't discussed this with him, I just CC him now to have his opinion. In fact for somecase I find useful to have useland application able to get notification from device without having devd running at all plus the devctl(4) manpage says: This design allows only one reader for /dev/devctl. This is not desirable in the long run, but will get a lot of hair out of this implementation. Maybe we should make this device a clonable device. that's why I didn't first spoke to Warner about this, which has been a mistake sorry about that. regards, Bapt pgpeWnhzKvcso.pgp Description: PGP signature
Re: [patch] turning devctl into a multiple openable device
On Wed, Nov 30, 2011 at 10:05:11AM -0500, John Baldwin wrote: On Wednesday, November 30, 2011 7:43:20 am Baptiste Daroussin wrote: Hi all, With the help of cognet, I wrote a patch to turn devctl into a multiple openable device, that mean that it will allow to open /dev/devctl in multiple programs, for example hald and everythings that want to receive notification from the device won't need to depend on haveing devd running. here is the patch: http://people.freebsd.org/~bapt/devctl_multi_open.diff Shouldn't devctl_queue_data_f() use the requested malloc() flags instead of hardcoding M_NOWAIT? This is an obvious fallback of holding mutex around the call to per_devctl_queue_data_f(), which caused the author a trouble to use M_WAITOK. Having n readers causes the patch to queue each message n times, that looks like a waste. 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. Also, I know that it was an intentional design decisison by Warner to have the multiplexing of devctl data done in userland via devd rather than in the kernel. (I think he envisioned devd providing a UNIX domain socket or some such for other daemons to use to listen to events.) Have you asked him about this change? And I fully agree that doing multiplexing in user mode is the right approach. Not least because you could apply some advanced access control and provide filtering for the consumers. pgpL1GaOrgCj6.pgp Description: PGP signature
Re: [patch] turning devctl into a multiple openable device
On Wed, Nov 30, 2011 at 05:46:36PM +0200, Kostik Belousov wrote: On Wed, Nov 30, 2011 at 10:05:11AM -0500, John Baldwin wrote: On Wednesday, November 30, 2011 7:43:20 am Baptiste Daroussin wrote: Hi all, With the help of cognet, I wrote a patch to turn devctl into a multiple openable device, that mean that it will allow to open /dev/devctl in multiple programs, for example hald and everythings that want to receive notification from the device won't need to depend on haveing devd running. here is the patch: http://people.freebsd.org/~bapt/devctl_multi_open.diff Shouldn't devctl_queue_data_f() use the requested malloc() flags instead of hardcoding M_NOWAIT? This is an obvious fallback of holding mutex around the call to per_devctl_queue_data_f(), which caused the author a trouble to use M_WAITOK. Having n readers causes the patch to queue each message n times, that looks like a waste. 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. Also, I know that it was an intentional design decisison by Warner to have the multiplexing of devctl data done in userland via devd rather than in the kernel. (I think he envisioned devd providing a UNIX domain socket or some such for other daemons to use to listen to events.) Have you asked him about this change? And I fully agree that doing multiplexing in user mode is the right approach. Not least because you could apply some advanced access control and provide filtering for the consumers. I agree that in most cases this is better, but being able to have multiple readers is anyway useful, having the futur libudev or alike not depends on devd running would be great imho. I have boxes that do not have devd and won't have it would be useless but run programs that needs to get notification for some hardware. I would love to remove devd on those boxes (they are boxes where the FS size is limited) regards, Bapt pgpL2XyYlWW0V.pgp Description: PGP signature
Re: [patch] turning devctl into a multiple openable device
On Wed, Nov 30, 2011 at 04:55:21PM +0100, Olivier Houchard wrote: On Wed, Nov 30, 2011 at 05:46:36PM +0200, Kostik Belousov wrote: On Wed, Nov 30, 2011 at 10:05:11AM -0500, John Baldwin wrote: On Wednesday, November 30, 2011 7:43:20 am Baptiste Daroussin wrote: Hi all, With the help of cognet, I wrote a patch to turn devctl into a multiple openable device, that mean that it will allow to open /dev/devctl in multiple programs, for example hald and everythings that want to receive notification from the device won't need to depend on haveing devd running. here is the patch: http://people.freebsd.org/~bapt/devctl_multi_open.diff Shouldn't devctl_queue_data_f() use the requested malloc() flags instead of hardcoding M_NOWAIT? This is an obvious fallback of holding mutex around the call to per_devctl_queue_data_f(), which caused the author a trouble to use M_WAITOK. Having n readers causes the patch to queue each message n times, that looks like a waste. Queuing the message only one time would require to somehow keep a state, to know which thread read which message, and figuring out when to free a message can be an headache. Given I don't think they'll be a lot of readers, I'm not sure it's worth the trouble. 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. pgpc9tuCURiWe.pgp Description: PGP signature
Re: [patch] turning devctl into a multiple openable device
On Wed, Nov 30, 2011 at 05:46:36PM +0200, Kostik Belousov wrote: On Wed, Nov 30, 2011 at 10:05:11AM -0500, John Baldwin wrote: On Wednesday, November 30, 2011 7:43:20 am Baptiste Daroussin wrote: Hi all, With the help of cognet, I wrote a patch to turn devctl into a multiple openable device, that mean that it will allow to open /dev/devctl in multiple programs, for example hald and everythings that want to receive notification from the device won't need to depend on haveing devd running. here is the patch: http://people.freebsd.org/~bapt/devctl_multi_open.diff Shouldn't devctl_queue_data_f() use the requested malloc() flags instead of hardcoding M_NOWAIT? This is an obvious fallback of holding mutex around the call to per_devctl_queue_data_f(), which caused the author a trouble to use M_WAITOK. Having n readers causes the patch to queue each message n times, that looks like a waste. Queuing the message only one time would require to somehow keep a state, to know which thread read which message, and figuring out when to free a message can be an headache. Given I don't think they'll be a lot of readers, I'm not sure it's worth the trouble. 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 ? Regards, Olivier ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [patch] turning devctl into a multiple openable device
On Wednesday 30 November 2011 13:43:20 Baptiste Daroussin wrote: Hi all, With the help of cognet, I wrote a patch to turn devctl into a multiple openable device, that mean that it will allow to open /dev/devctl in multiple programs, for example hald and everythings that want to receive notification from the device won't need to depend on haveing devd running. here is the patch: http://people.freebsd.org/~bapt/devctl_multi_open.diff regards, bapt Comment: Is the track-close flag set for the devfs sw? --HPS ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [patch] turning devctl into a multiple openable device
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 ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
