Date: Fri, 22 Oct 2004 00:10:26 -0700 From: John-Mark Gurney <gurney_j@resnet.uoregon.edu> To: Doug Ambrisko <ambrisko@ambrisko.com> Cc: current@freebsd.org Subject: Re: lio_listio fixes and adding kqueue notification patch Message-ID: <20041022071026.GW22681@funkthat.com> In-Reply-To: <200410212348.i9LNmucN008525@ambrisko.com> References: <20041021225201.GT22681@funkthat.com> <200410212348.i9LNmucN008525@ambrisko.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Doug Ambrisko wrote this message on Thu, Oct 21, 2004 at 16:48 -0700: > John-Mark Gurney writes: > | Doug Ambrisko wrote this message on Wed, Oct 20, 2004 at 12:10 -0700: > | > Here's a patch to fix some lio_listio bugs and add kqueue support to it. > | > BTW kqueue & aio_read/write is broken and that seems to have happened > | > during the locking changes. I could use some more testers for the > | > signal causes. > | > > | > http://www.ambrisko.com/doug/listio_kqueue/listio_kqueue.patch > | > > | > The summary is: > | > kqueue changes: > | > - kqueue locking removed/broke knote_remove (transformed into > | > knlist_clear??) which no longer deletes and clean up the > | > the knotes so they are left hanging around and can break the > | > next transfer since they are already triggered etc. > | > - add knlist_delete to restore knote_remove functionality to > | > totally take it of the system. When you do an aio_return > | > (which implies aio_free_entry) the kevent can never trigger > | > again since the context is lost. > | > | why did you copy so much of knlist_clear into knlist_delete? why not > | expand knlist_clear to optionally do the delete instead of duplicating > | the code? Also, why do you duplicate all the logic in knote_drop into > | knlist_delete? why not make a knote_drop_locked function, and call it > | from knlist_delete? > > Since I'm not an expert on this stuff and I didn't want to break > the existing code ... such as happened when the locking was done. > When I initially did some change I ran into locking issues that were not > obvious to figure out. I'm willing to adjust that as you suggest. Didn't > want to spend a lot of time it and deal with merge conflicts. > This is way I asked for a review to work toward a proper solution. np... > | I'll send you a kqueue manpage that I've written so far for your > | review... > | > | I don't like removing knotes completely w/o it being returned to the > | userland unless there is good reason (like the user did a close on an > | fd and expects the knote to go away)... some people (like me) use knote's > | in the kernel to keep track of resources... so, will it be documented > | that this is what happens? > > When an aio operation is complete it is dead and it can't happen > again. The aio_return is acknowledgement that the transaction is > complete and can't happen again like a close (the only thing different > from a close is that other I/O can still happen on the fd. If you leave it > around then the next aio op gets the prior knote done when it hasn't > done anything. Bad things happen. This is documented in the aio > usage stuff. Ok, we should mark the kevent as EV_ONESHOT, then once the event has finished, we could simply detached the knote, and not need to clean it up anymore... > | also, why arey ou adding an splbio call? spl calls in 5.x and 6.x are > | if def'd to nothing.. it'd be better to add an XXX comment about requiring > | locking... or GIANT_REQUIRED; > > Consistancy for back-porting to 4.X ... it already has them. Might as > well make it correct in the one minor case that I changed it. It is > a minor point. Future work would be to make it giant free like Alfred > has started. I brought this code up to 5.X via diff and patch > since I did this implementation under 4.X. So that no big thing > to change if absolutely needed. > > | > - hack, remove the mtx_assert in knlist_empty so it can be used > | > to see if there are kqueue's on an aio task. > | > | I don't like this.. The problem I have with this is that between the > | time that knlist_empty returns, there could be a knote added... so any > | decision based upon that check is stale? If you know you aren't going > | to be adding anymore knlist_clear (and by the copying knlist_delete) is > | safe to call w/o having to check knlist_empty.. They aren't going to > | be doing much more work that knlist_empty isn't going to do... (and > | prevents you having to remove the mtx_assert in knlist_empty).. > > How are the knotes added since the aio call makes them when > you submit an aio I/O request and only lives until acknowledged by > aio_return. So aio creates, registers and clears the kevent. The user ahh, ok. yeh, I see that now... > then listens to them. If there are no knotes then you are going to > use the signal method of delivery. Mostly it is a panic saver of > which I hit a few. Strictly it isn't needed but it doesn't hurt. > kqueue & aio is special. Have you read the EVFILT_AIO part of > man kqueue? It has to be registered on the aio operation or you actually, we need to cut out the second paragraph.. hmmm. this is funny, EV_FLAG1 is not restricted to the kernel, which means that the userland can register an AIO request... > might not get it registered fast enough before it returns. If you > happen to register a kevent on the aio op. outside the aio commands > you might never get notified since the check is only done on I/O > completion which may have already occured. So this isn't a problem. yeh, definately need to fix this... > | [aio stuff deleted] > | > | can't really comment on the aio stuff since I don't know it that > | well.. > > Which is why you broke it and it doesn't work now :-( The lio_listio > is pretty busted as is and doesn't work reliably regardless of kevent. > > I hope we can work to a solution that is stable and works well with aio. > The guts are here it more of an issue of how to best integrate it. sure... we'll find a solution... do you have some good test programs? -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20041022071026.GW22681>