Re: Greetings... a patch I would like your comments on...
Daniel:
Well it may be ugly, but it gets even
uglier on how you have to pass the mtx down
to the kernel with the cond wait variables...
One can do it.. but it will take some additional
changes to the kevent structure which breaks the ABI... which
I was not going to venture down that path :-)
Hmm I suppose one might be able to abuse udata. Right
now the incoming "value" is in the data (the value that is checked
before putting you on the umtx sleep queue).
We could, pass the mtx into the "udata" field but this rather breaks
the definition of udata:
"Opaque user-defined value passed through the kernel unchanged"
Now I suppose if you placed the MTX pointer there that you are
NOT exactly changing it ... but you WOULD be changing the value
of it (as you unlock the mutex)..
I will work on getting this first set of changes into the kernel. If
we don't abuse udata, then the only choice would be to add some
additional
"data" element.. or maybe as Ivan suggested a "blob" which would
still mean an ABI change since you would have to include a size in
the blob... hmmm unless we did "data" is a pointer and flags is the
size.. But this really seems hackish .. sigh ..
R
On Jan 22, 2010, at 9:58 AM, Daniel Eischen wrote:
This is ugly from the userland point of view -- if you need
this, I would encompass everything in a kq event object
thingie. As in:
/*
* Create an object containing everything you need to wait
* or signal kqueue events.
*/
kq = kqueue();
kq_obj = kq_create(kq, ...);
kq_lock(&kq_obj);
while (!P)
{
/* Atomically unlocks kq_obj and blocks. */
nevents = kq_wait(&kq_obj, ...);
/* When you wakeup, kq_obj is locked again. */
}
do_work(); /* Wouldn't you want to unlock before this? */
kq_unlock(&kq_obj);
/* From another thread, you could do: */
kq_lock(&kq_obj);
if (kq_waiters(&kq_obj) > 0)
kq_signal(&kq_obj); /* or kq_broadcast() */
kq_unlock(&kq_obj);
--
DE
--
Randall Stewart
803-317-4952 (cell)
803-345-0391(direct)
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Greetings... a patch I would like your comments on...
On Jan 22, 2010, at 9:25 AM, Jilles Tjoelker wrote:
On Fri, Jan 22, 2010 at 07:55:47AM -0800, Randall Stewart wrote:
On Jan 22, 2010, at 7:33 AM, Ivan Voras wrote:
On 01/22/10 16:10, Ed Schouten wrote:
* Ivan Voras wrote:
This is a good and useful addition! I think Windows has
implemented a
generalization of this (called "wait objects" or something like
that),
which effectively allows a select()- (or in this case kqueue())-
like
syscall to wait on both file descriptors and condvars (as well as
probably other MS-style objects). It's useful for multiplexing
events
for dissimilar sources.
NtWaitForSingleObject(), NtWaitForMultipleObjects(), etc. :-)
Yes, I was thinking about WaitForMultipleObjects() - I sometimes
wished I had it in FreeBSD :)
I think the hackers@ side of the thread is missing the original link
to the patch file offered for review, so here it is:
http://people.freebsd.org/~rrs/kque_umtx.patch
Cool.
My kqueue-fu level is too low to be really useful here but from what
I've read it looks like a logical and even reasonably clean way of
doing it.
thanks it made sense to me ;-)
If I read the comment at filt_umtxattach() correctly, in the best
case you would need an extension to the kevent structure to add more
fields like data & udata (for passing values back and forth between
userland and kernel). I agree with this - it would be very
convenient for some future purposes (like file modification
notification) if the kernel filter could both accept and return a
struct of data from/to the userland.
Yeah, more arguments inside the kevent would allow me to add the
COND_CV_WAIT* where a lock and condition are passed
in as well... But I was hesitant to add more than was already there
since doing
so would cause ABI ripples that I did not want to face.
I don't think passing the lock is needed.
Traditional way (error handling omitted):
pthread_mutex_lock(&M);
while (!P)
pthread_cond_wait(&C, &M);
do_work();
pthread_mutex_unlock(&M);
The thread must be registered as a waiter before unlocking the mutex,
otherwise a wakeup could be lost.
Possible kqueue way (error handling omitted):
kq = kqueue();
pthread_mutex_lock(&M);
while (!P)
{
pthread_cond_kqueue_register_wait_np(&C, kq);
pthread_mutex_unlock(&M);
nevents = kevent(kq, NULL, 0, events, 1, NULL);
... handle other events ...
pthread_mutex_lock(&M);
}
do_work();
pthread_mutex_unlock(&M);
close(kq);
True, we are doing this at my day job.. I had just
noticed a UMTX_CV_WAIT type wait.. and that one passes
in a lock to unlock..
I just wanted it clear that this can't do that one.. you
have to do something (like what you outlined) to deal
with the mutex...
To avoid lost wakeup, the kqueue must be registered as a waiter before
unlocking the mutex. Once it has been registered, it is safe to unlock
the mutex as the kqueue will store the fact that the condition has
been
signalled. Hence, pthread_cond_kqueue_register_wait_np() or however it
will be named does not need the mutex explicitly.
kevent() needs to return some sort of identifier of C, possibly via
kevent.udata. In that case the hack with fflags and data remains
needed.
Hmm yeah right now the return is really just the ident which
tells you the address waited on. We probably will need something
more to tie things together when the pthread side of this is built.
Registering multiple condvars in one function call could be nasty,
as it
requires all the mutexes to be locked simultaneously.
On the other hand if you do want registration and waiting in the same
function, like kqueue can do with file descriptors, then it is
needed to
pass all the locks so they can be unlocked after registering and
before
waiting. I think this results in a rather complicated API though.
Yep.. it leads to a twisty passage of fun ;-)
Adding a mutex to a kqueue probably means that mutexes are used
wrongly:
they should not be locked for so long that it is desirable to wait for
one together with other objects (pthread_mutex_timedlock() is meant
as a
fail-safe).
Semaphores could also be waited for via kqueue, probably via a version
of sem_trywait() that registers the kqueue as a waiter. Once this is
signalled, the thread can call the function again to obtain the
semaphore or register again if another thread obtained the semaphore
first.
True.. I had not thought of doing that.. I was just more
interested in waiting on an fd and a cond... :-)
R
--
Jilles Tjoelker
--
Randall Stewart
803-317-4952 (cell)
803-345-0391(direct)
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Greetings... a patch I would like your comments on...
On Jan 22, 2010, at 8:04 AM, Ivan Voras wrote: On 01/22/10 16:55, Randall Stewart wrote: If I read the comment at filt_umtxattach() correctly, in the best case you would need an extension to the kevent structure to add more fields like data & udata (for passing values back and forth between userland and kernel). I agree with this - it would be very convenient for some future purposes (like file modification notification) if the kernel filter could both accept and return a struct of data from/to the userland. Yeah, more arguments inside the kevent would allow me to add the COND_CV_WAIT* where a lock and condition are passed in as well... But I was hesitant to add more than was already there since doing so would cause ABI ripples that I did not want to face. Yes, this should be done carefully; just adding more "data" and "udata" fields will postpone the problem to when someone else needs one more field to make his idea working - a memory blob should probably be the way to go. I plan on committing this to head if I don't get strong "you idiot you did it wrong" comments ;-) Hmmm, something just occured to me: why did you name the event / filter "EVFILT_KQUEUE"? Why not something like "EVFILT_UMTX" or "EVFLT_COND"? Yep.. has I am just sitting down to write my "super crusher" test case I realize .. what was I smoking when I said EVFILT_KQUEUE.. thats got to change.. I think maybe EVFILT_UMTX_COND is best.. UMTX might let you think you could send in a lock or something else.. You said you didn't make the actual connection to the userland pthead_* API yet - how did you test it? Actually I am using just raw umtx itself. For just quick testing a _thr_umtx_wake((u_long *)addr, count); Works.. Or one can even dig in and do the actual syscall.. _umtx_op().. but that requires a lot more arguments ;-) I need to think on integrating it into pthread*.. but we actully have at work our own mutex stuff for apps that is in user space and is a bit faster (quite a bit)... and it uses the umtx stuff directly ;-) R ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org " -- Randall Stewart 803-317-4952 (cell) 803-345-0391(direct) ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Greetings... a patch I would like your comments on...
Ivan: A couple of comments... On Jan 22, 2010, at 7:33 AM, Ivan Voras wrote: On 01/22/10 16:10, Ed Schouten wrote: * Ivan Voras wrote: This is a good and useful addition! I think Windows has implemented a generalization of this (called "wait objects" or something like that), which effectively allows a select()- (or in this case kqueue())-like syscall to wait on both file descriptors and condvars (as well as probably other MS-style objects). It's useful for multiplexing events for dissimilar sources. NtWaitForSingleObject(), NtWaitForMultipleObjects(), etc. :-) Yes, I was thinking about WaitForMultipleObjects() - I sometimes wished I had it in FreeBSD :) I think the hackers@ side of the thread is missing the original link to the patch file offered for review, so here it is: http://people.freebsd.org/~rrs/kque_umtx.patch My kqueue-fu level is too low to be really useful here but from what I've read it looks like a logical and even reasonably clean way of doing it. thanks it made sense to me ;-) If I read the comment at filt_umtxattach() correctly, in the best case you would need an extension to the kevent structure to add more fields like data & udata (for passing values back and forth between userland and kernel). I agree with this - it would be very convenient for some future purposes (like file modification notification) if the kernel filter could both accept and return a struct of data from/to the userland. Yeah, more arguments inside the kevent would allow me to add the COND_CV_WAIT* where a lock and condition are passed in as well... But I was hesitant to add more than was already there since doing so would cause ABI ripples that I did not want to face. I plan on committing this to head if I don't get strong "you idiot you did it wrong" comments ;-) R ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org " -- Randall Stewart 803-317-4952 (cell) 803-345-0391(direct) ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Greetings... a patch I would like your comments on...
Ivan: Ok, over we go ;-) I do want to add this into Head here eventually so if you happen to have an interest in umtx or kqueue you may want to take a close look at this patch ;-) R On Jan 22, 2010, at 5:27 AM, Ivan Voras wrote: 2010/1/22 Randall Stewart : All: I have put together a patch against head that I would like your opinion of. So first what does it do? Well one thing I thought lacking in the kernel was the ability to send a cond event (umtx_cond) to a thread that was waiting on a kqueue... So the rough idea is I have N fd's and other things I am watching but I would also like a local thread (maybe remote if the umtx_cond_t is in shared memory) to be able to wake me up as well. This is a good and useful addition! I think Windows has implemented a generalization of this (called "wait objects" or something like that), which effectively allows a select()- (or in this case kqueue())-like syscall to wait on both file descriptors and condvars (as well as probably other MS-style objects). It's useful for multiplexing events for dissimilar sources. But you will probably soon receive a message to take this discussion to hack...@freebsd.org, and I agree :) ---------- Randall Stewart 803-317-4952 (cell) 803-345-0391(direct) ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Greetings... a patch I would like your comments on...
Lets try that again with the right email subscribed to hackers ;-) R On Jan 22, 2010, at 6:43 AM, Randall Stewart wrote: Ivan: Ok, over we go ;-) I do want to add this into Head here eventually so if you happen to have an interest in umtx or kqueue you may want to take a close look at this patch ;-) R On Jan 22, 2010, at 5:27 AM, Ivan Voras wrote: 2010/1/22 Randall Stewart : All: I have put together a patch against head that I would like your opinion of. So first what does it do? Well one thing I thought lacking in the kernel was the ability to send a cond event (umtx_cond) to a thread that was waiting on a kqueue... So the rough idea is I have N fd's and other things I am watching but I would also like a local thread (maybe remote if the umtx_cond_t is in shared memory) to be able to wake me up as well. This is a good and useful addition! I think Windows has implemented a generalization of this (called "wait objects" or something like that), which effectively allows a select()- (or in this case kqueue())-like syscall to wait on both file descriptors and condvars (as well as probably other MS-style objects). It's useful for multiplexing events for dissimilar sources. But you will probably soon receive a message to take this discussion to hack...@freebsd.org, and I agree :) ---------- Randall Stewart 803-317-4952 (cell) 803-345-0391(direct) ---------- Randall Stewart 803-317-4952 (cell) 803-345-0391(direct) ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: cvs commit: src/sys/netinet sctp_asconf.c sctp_input.c sctp_output.c sctp_pcb.c sctp_timer.c sctp_usrreq.c sctputil.c
Max Laier wrote: On Thursday 16 August 2007, you wrote: rrs 2007-08-16 01:51:22 UTC FreeBSD src repository Modified files: sys/netinet sctp_asconf.c sctp_input.c sctp_output.c sctp_pcb.c sctp_timer.c sctp_usrreq.c sctputil.c Log: - Remove extra comment for 7.0 (no GIANT here). - Remove unneeded WLOCK/UNLOCK of inp for getting TCB lock. - Fix panic that may occur when freeing an assoc that has partial delivery in progress (may dereference null socket pointer when queuing partial delivery aborted notification) - Some spacing and comment fixes. - Fix address add handling to clear cached routes and source addresses when peer acks the add in case the routing table changes. Approved by:[EMAIL PROTECTED] (Bruce Mah) Revision ChangesPath 1.24 +49 -2 src/sys/netinet/sctp_asconf.c 1.56 +3 -7 src/sys/netinet/sctp_input.c 1.49 +48 -41src/sys/netinet/sctp_output.c 1.52 +1 -4 src/sys/netinet/sctp_pcb.c 1.26 +0 -2 src/sys/netinet/sctp_timer.c 1.42 +1 -2 src/sys/netinet/sctp_usrreq.c 1.55 +4 -5 src/sys/netinet/sctputil.c This doesn't include the memset reversion fix on -hackers recently. Just makeing sure it's not forgotten as I happen to remember just now. No.. this patch was one I submitted 8/7... I did not have the memset patch then.. The patch you speak of is already incorporated into the main CVS tree we (the sctp team) use to support the three O/S's we work on (FreeBSD/MAC-OSX/IOS-XR) The next patch that is building, patch 15, currently looks as follows: * bash-stewlap: cat list_of_updates - Fix address add handling to clear cached routes and source addresses when peer acks the add in case the routing table changes. - Fix sctp_lower_sosend to send shutdown chunk for mbuf send case when sndlen = 0 and sinfoflag = SCTP_EOF - Fix sctp_lower_sosend for SCTP_ABORT mbuf send case with null data, So that it does not send the "null" data mbuf out and cause it to get freed twice. - Fix so auto-asconf sysctl actually effect the socket's asconf state. - Do not allow SCTP_AUTO_ASCONF option to be used on subset bound sockets. - Memset bug in sctp_output.c (arguments were reversed) submitted found and reported by Dave Jones ([EMAIL PROTECTED]). - PD-API point needs to be invoked >= not just > to conform to socket api draft this fixes sctp_indata.c in the two places need to be >=. ** As you can see Dave's patch is there.. but I have not submitted this yet. The next SCTP-Interop starts Sunday (I am in Japan helping setup for this now) and I think it is best I wait to see what other bugs surface at the interop before queuing the next patch to re... Unless you want me to speed the sending of this up.. if so I can start my make universe builds and begin patch prep.. but if not.. I will wait .. let me know if you want me to do so.. :-D R -- Randall Stewart NSSTG - Cisco Systems Inc. 803-345-0369 803-317-4952 (cell) ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: memset bugs.
Thanks for the pointer...
Julian and Sam also sent this to me on the SCTP side.
The local CVS repository on lakerest.net now has this fix
in it.. and others... I have added this to the queue to
go in to patchset 15.. (I am still waiting on re for patchset 14).
R
Dag-Erling Smørgrav wrote:
Dave Jones <[EMAIL PROTECTED]> writes:
A grep I crafted to pick up on some common bugs happened upon
a copy of the FreeBSD CVS tree that I happened to have handy
and found the bugs below where the 2nd & 3rd arguments to
memset calls have been swapped.
[...]
--- src/sys/netinet/sctp_output.c~ 2007-08-14 15:44:11.0 -0400
+++ src/sys/netinet/sctp_output.c 2007-08-14 15:44:27.0 -0400
@@ -6331,7 +6331,7 @@ out_gu:
rcv_flags |= SCTP_DATA_UNORDERED;
}
/* clear out the chunk before setting up */
- memset(chk, sizeof(*chk), 0);
+ memset(chk, 0, sizeof(*chk));
chk->rec.data.rcv_flags = rcv_flags;
if (SCTP_BUF_IS_EXTENDED(sp->data)) {
chk->copy_by_ref = 1;
Pointy hat to [EMAIL PROTECTED]
--- src/usr.sbin/nscd/agents/services.c~2007-08-14 15:44:33.0
-0400
+++ src/usr.sbin/nscd/agents/services.c 2007-08-14 15:44:41.0 -0400
@@ -171,7 +171,7 @@ services_lookup_func(const char *key, si
if (size > 0) {
proto = (char *)malloc(size + 1);
assert(proto != NULL);
- memset(proto, size + 1, 0);
+ memset(proto, 0, size + 1);
memcpy(proto, key + sizeof(enum nss_lookup_type) +
sizeof(int), size);
}
--- src/usr.sbin/cached/agents/services.c~ 2007-08-14 15:44:45.0
-0400
+++ src/usr.sbin/cached/agents/services.c 2007-08-14 15:44:52.0
-0400
@@ -171,7 +171,7 @@ services_lookup_func(const char *key, si
if (size > 0) {
proto = (char *)malloc(size + 1);
assert(proto != NULL);
- memset(proto, size + 1, 0);
+ memset(proto, 0, size + 1);
memcpy(proto, key + sizeof(enum nss_lookup_type) +
sizeof(int), size);
}
These two are actually the same file - cached is in the process of being
renamed to nscd. Pointy hat to [EMAIL PROTECTED]
--- src/contrib/gdb/gdb/std-regs.c~ 2007-08-14 15:44:56.0 -0400
+++ src/contrib/gdb/gdb/std-regs.c 2007-08-14 15:45:22.0 -0400
@@ -61,7 +61,7 @@ value_of_builtin_frame_reg (struct frame
val = allocate_value (builtin_type_frame_reg);
VALUE_LVAL (val) = not_lval;
buf = VALUE_CONTENTS_RAW (val);
- memset (buf, TYPE_LENGTH (VALUE_TYPE (val)), 0);
+ memset (buf, 0, TYPE_LENGTH (VALUE_TYPE (val)));
/* frame.base. */
if (frame != NULL)
ADDRESS_TO_POINTER (builtin_type_void_data_ptr, buf,
@@ -87,7 +87,7 @@ value_of_builtin_frame_fp_reg (struct fr
struct value *val = allocate_value (builtin_type_void_data_ptr);
char *buf = VALUE_CONTENTS_RAW (val);
if (frame == NULL)
- memset (buf, TYPE_LENGTH (VALUE_TYPE (val)), 0);
+ memset (buf, 0, TYPE_LENGTH (VALUE_TYPE (val)));
else
ADDRESS_TO_POINTER (builtin_type_void_data_ptr, buf,
get_frame_base_address (frame));
@@ -105,7 +105,7 @@ value_of_builtin_frame_pc_reg (struct fr
struct value *val = allocate_value (builtin_type_void_data_ptr);
char *buf = VALUE_CONTENTS_RAW (val);
if (frame == NULL)
- memset (buf, TYPE_LENGTH (VALUE_TYPE (val)), 0);
+ memset (buf, 0, TYPE_LENGTH (VALUE_TYPE (val)));
else
ADDRESS_TO_POINTER (builtin_type_void_data_ptr, buf,
get_frame_pc (frame));
--- src/contrib/gdb/gdb/remote.c~ 2007-08-14 15:45:25.0 -0400
+++ src/contrib/gdb/gdb/remote.c2007-08-14 15:45:37.0 -0400
@@ -3463,7 +3463,7 @@ remote_store_registers (int regnum)
{
int i;
regs = alloca (rs->sizeof_g_packet);
-memset (regs, rs->sizeof_g_packet, 0);
+memset (regs, 0, rs->sizeof_g_packet);
for (i = 0; i < NUM_REGS + NUM_PSEUDO_REGS; i++)
{
struct packet_reg *r = &rs->regs[i];
These should go upstream to the gdb maintainers ([EMAIL PROTECTED]).
DES
--
Randall Stewart
NSSTG - Cisco Systems Inc.
803-345-0369 803-317-4952 (cell)
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
