> On 21 Jan 2015, at 5:50 am, Simon Mages <mages.si...@googlemail.com> wrote: > > Sorry, i did not test the Patch well enough. > > It is still broken, but in a different way. > > I think tedu forgot in his patch to reset d->bd_rdStart. From my point > of view it > should be zero after sleeping in this case. Because if you read again after a > successful read the timeout wont be processed because of: > >> /* >> * If there's a timeout, bd_rdStart is tagged when we start the read. >> * we can then figure out when we're done reading. >> */ >> if (d->bd_rtout != -1 && d->bd_rdStart == 0) >> d->bd_rdStart = ticks; >> else >> d->bd_rdStart = 0; > > And zero is all the time smaller then the elapsed time in the second read. > > This would fix it: > if (elapsed < d->bd_rtout) { > error = tsleep(d, PRINET|PCATCH, "bpf", > d->bd_rtout - elapsed); > + d->bd_rdStart = 0; > } else > error = EWOULDBLOCK;
yes, that makes sense to me. ill commit it when im back at work (tuesday the 27th around 11am gmt+10) unless someone objects. or beats me too it. dlg > > BR > Simon > > 2015-01-07 9:11 GMT+01:00, Simon Mages <mages.si...@googlemail.com>: >> I tested the patch and its working. >> >> I have a small test program already. I create a regression test with it. >> I'll post the diff here. >> Am 06.01.2015 04:19 schrieb "Philip Guenther" <guent...@gmail.com>: >> >>> [(@#*$&(*# control-enter keybinding] >>> >>> On Mon, Jan 5, 2015 at 7:15 PM, Philip Guenther <guent...@gmail.com> >>> wrote: >>>> On Mon, Jan 5, 2015 at 11:01 AM, Ted Unangst <t...@tedunangst.com> >>> wrote: >>>> ... >>>>> In the regular timeout case, I'm not sure what you're changing. There >>>>> is a problem here though. If we're already close to the timeout >>>>> expiring, we shouldn't sleep the full timeout, only the time left >>>>> since we began the read. >>> >>> Yes, that was what I was trying to convey in my reply to Mages's >>> earlier post on this bpf issue. >>> >>> Your diff looks correct to me, though untested. >>> >>> Mages, do you have code this can be tested against? Is there >>> something you could contribute to form a regress test we could place >>> under /usr/src/regress/net/ to verify that we got this right and to >>> catch breakage in the future? >>> >>> >>> Philip Guenther >>> >> >