* Poll ring behavior broken by f0c5c54945ae92a00cdbb43bdf3abaeab6bd3a23 @ 2020-08-21 0:46 Glauber Costa 2020-08-21 1:57 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Glauber Costa @ 2020-08-21 0:46 UTC (permalink / raw) To: io-uring, axboe, xiaoguang.wang I have just noticed that the commit in $subject broke the behavior I introduced in bf3aeb3dbbd7f41369ebcceb887cc081ffff7b75 In this commit, I have explained why and when it does make sense to enter the ring if there are no sqes to submit. I guess one could argue that in that case one could call the system call directly, but it is nice that the application didn't have to worry about that, had to take no conditionals, and could just rely on io_uring_submit as an entry point. Since the author is the first to say in the patch that the patch may not be needed, my opinion is that not only it is not needed but in fact broke applications that relied on previous behavior on the poll ring. Can we please revert? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Poll ring behavior broken by f0c5c54945ae92a00cdbb43bdf3abaeab6bd3a23 2020-08-21 0:46 Poll ring behavior broken by f0c5c54945ae92a00cdbb43bdf3abaeab6bd3a23 Glauber Costa @ 2020-08-21 1:57 ` Jens Axboe 2020-08-21 2:24 ` Glauber Costa 0 siblings, 1 reply; 6+ messages in thread From: Jens Axboe @ 2020-08-21 1:57 UTC (permalink / raw) To: Glauber Costa, io-uring, xiaoguang.wang On 8/20/20 6:46 PM, Glauber Costa wrote: > I have just noticed that the commit in $subject broke the behavior I > introduced in > bf3aeb3dbbd7f41369ebcceb887cc081ffff7b75 > > In this commit, I have explained why and when it does make sense to > enter the ring if there are no sqes to submit. > > I guess one could argue that in that case one could call the system > call directly, but it is nice that the application didn't have to > worry about that, had to take no conditionals, and could just rely on > io_uring_submit as an entry point. > > Since the author is the first to say in the patch that the patch may > not be needed, my opinion is that not only it is not needed but in > fact broke applications that relied on previous behavior on the poll > ring. > > Can we please revert? Yeah let's just revert it for now. Any chance you can turn this into a test case for liburing? Would help us not break this in the future. -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Poll ring behavior broken by f0c5c54945ae92a00cdbb43bdf3abaeab6bd3a23 2020-08-21 1:57 ` Jens Axboe @ 2020-08-21 2:24 ` Glauber Costa 2020-08-21 3:42 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Glauber Costa @ 2020-08-21 2:24 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, xiaoguang.wang On Thu, Aug 20, 2020 at 9:57 PM Jens Axboe <[email protected]> wrote: > > On 8/20/20 6:46 PM, Glauber Costa wrote: > > I have just noticed that the commit in $subject broke the behavior I > > introduced in > > bf3aeb3dbbd7f41369ebcceb887cc081ffff7b75 > > > > In this commit, I have explained why and when it does make sense to > > enter the ring if there are no sqes to submit. > > > > I guess one could argue that in that case one could call the system > > call directly, but it is nice that the application didn't have to > > worry about that, had to take no conditionals, and could just rely on > > io_uring_submit as an entry point. > > > > Since the author is the first to say in the patch that the patch may > > not be needed, my opinion is that not only it is not needed but in > > fact broke applications that relied on previous behavior on the poll > > ring. > > > > Can we please revert? > > Yeah let's just revert it for now. Any chance you can turn this into > a test case for liburing? Would help us not break this in the future. would be my pleasure. Biggest issue is that poll mode really only works with ext4 and xfs as far as I know. That may mean it won't get as much coverage, but maybe that's not relevant. > > -- > Jens Axboe > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Poll ring behavior broken by f0c5c54945ae92a00cdbb43bdf3abaeab6bd3a23 2020-08-21 2:24 ` Glauber Costa @ 2020-08-21 3:42 ` Jens Axboe 2020-08-21 13:55 ` Glauber Costa 0 siblings, 1 reply; 6+ messages in thread From: Jens Axboe @ 2020-08-21 3:42 UTC (permalink / raw) To: Glauber Costa; +Cc: io-uring, xiaoguang.wang On 8/20/20 8:24 PM, Glauber Costa wrote: > On Thu, Aug 20, 2020 at 9:57 PM Jens Axboe <[email protected]> wrote: >> >> On 8/20/20 6:46 PM, Glauber Costa wrote: >>> I have just noticed that the commit in $subject broke the behavior I >>> introduced in >>> bf3aeb3dbbd7f41369ebcceb887cc081ffff7b75 >>> >>> In this commit, I have explained why and when it does make sense to >>> enter the ring if there are no sqes to submit. >>> >>> I guess one could argue that in that case one could call the system >>> call directly, but it is nice that the application didn't have to >>> worry about that, had to take no conditionals, and could just rely on >>> io_uring_submit as an entry point. >>> >>> Since the author is the first to say in the patch that the patch may >>> not be needed, my opinion is that not only it is not needed but in >>> fact broke applications that relied on previous behavior on the poll >>> ring. >>> >>> Can we please revert? >> >> Yeah let's just revert it for now. Any chance you can turn this into >> a test case for liburing? Would help us not break this in the future. > > would be my pleasure. > > Biggest issue is that poll mode really only works with ext4 and xfs as > far as I know. That may mean it won't get as much coverage, but maybe > that's not relevant. And raw nvme too, of course. But I'd say coverage is pretty decent with those two, in reality that's most likely what people would use for polling anyway. So not too concerned about that, and it'll hit multiple items in my test suite. I reverted the change manually, it didn't revert cleanly. Please test current -git, thanks! -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Poll ring behavior broken by f0c5c54945ae92a00cdbb43bdf3abaeab6bd3a23 2020-08-21 3:42 ` Jens Axboe @ 2020-08-21 13:55 ` Glauber Costa 2020-08-21 14:36 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Glauber Costa @ 2020-08-21 13:55 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, xiaoguang.wang On Thu, Aug 20, 2020 at 11:42 PM Jens Axboe <[email protected]> wrote: > > On 8/20/20 8:24 PM, Glauber Costa wrote: > > On Thu, Aug 20, 2020 at 9:57 PM Jens Axboe <[email protected]> wrote: > >> > >> On 8/20/20 6:46 PM, Glauber Costa wrote: > >>> I have just noticed that the commit in $subject broke the behavior I > >>> introduced in > >>> bf3aeb3dbbd7f41369ebcceb887cc081ffff7b75 > >>> > >>> In this commit, I have explained why and when it does make sense to > >>> enter the ring if there are no sqes to submit. > >>> > >>> I guess one could argue that in that case one could call the system > >>> call directly, but it is nice that the application didn't have to > >>> worry about that, had to take no conditionals, and could just rely on > >>> io_uring_submit as an entry point. > >>> > >>> Since the author is the first to say in the patch that the patch may > >>> not be needed, my opinion is that not only it is not needed but in > >>> fact broke applications that relied on previous behavior on the poll > >>> ring. > >>> > >>> Can we please revert? > >> > >> Yeah let's just revert it for now. Any chance you can turn this into > >> a test case for liburing? Would help us not break this in the future. > > > > would be my pleasure. > > > > Biggest issue is that poll mode really only works with ext4 and xfs as > > far as I know. That may mean it won't get as much coverage, but maybe > > that's not relevant. > > And raw nvme too, of course. But I'd say coverage is pretty decent with > those two, in reality that's most likely what people would use for > polling anyway. So not too concerned about that, and it'll hit multiple > items in my test suite. > > I reverted the change manually, it didn't revert cleanly. Please test > current -git, thanks! > Just tested (through a new unit test) and it works, thanks. I wrote a unit test that works on HEAD but not on HEAD^. However you will have to excuse my lack of manners, but in my new work account I can't have app passwords for GSuite so I am unable to git-send-email it without talking to a host of corporate IT people... I'll have to send a ... urgh... PR > -- > Jens Axboe > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Poll ring behavior broken by f0c5c54945ae92a00cdbb43bdf3abaeab6bd3a23 2020-08-21 13:55 ` Glauber Costa @ 2020-08-21 14:36 ` Jens Axboe 0 siblings, 0 replies; 6+ messages in thread From: Jens Axboe @ 2020-08-21 14:36 UTC (permalink / raw) To: Glauber Costa; +Cc: io-uring, xiaoguang.wang On 8/21/20 7:55 AM, Glauber Costa wrote: > On Thu, Aug 20, 2020 at 11:42 PM Jens Axboe <[email protected]> wrote: >> >> On 8/20/20 8:24 PM, Glauber Costa wrote: >>> On Thu, Aug 20, 2020 at 9:57 PM Jens Axboe <[email protected]> wrote: >>>> >>>> On 8/20/20 6:46 PM, Glauber Costa wrote: >>>>> I have just noticed that the commit in $subject broke the behavior I >>>>> introduced in >>>>> bf3aeb3dbbd7f41369ebcceb887cc081ffff7b75 >>>>> >>>>> In this commit, I have explained why and when it does make sense to >>>>> enter the ring if there are no sqes to submit. >>>>> >>>>> I guess one could argue that in that case one could call the system >>>>> call directly, but it is nice that the application didn't have to >>>>> worry about that, had to take no conditionals, and could just rely on >>>>> io_uring_submit as an entry point. >>>>> >>>>> Since the author is the first to say in the patch that the patch may >>>>> not be needed, my opinion is that not only it is not needed but in >>>>> fact broke applications that relied on previous behavior on the poll >>>>> ring. >>>>> >>>>> Can we please revert? >>>> >>>> Yeah let's just revert it for now. Any chance you can turn this into >>>> a test case for liburing? Would help us not break this in the future. >>> >>> would be my pleasure. >>> >>> Biggest issue is that poll mode really only works with ext4 and xfs as >>> far as I know. That may mean it won't get as much coverage, but maybe >>> that's not relevant. >> >> And raw nvme too, of course. But I'd say coverage is pretty decent with >> those two, in reality that's most likely what people would use for >> polling anyway. So not too concerned about that, and it'll hit multiple >> items in my test suite. >> >> I reverted the change manually, it didn't revert cleanly. Please test >> current -git, thanks! >> > > Just tested (through a new unit test) and it works, thanks. > > I wrote a unit test that works on HEAD but not on HEAD^. Perfect, thanks! > However you will have to excuse my lack of manners, but in my new work > account I can't have app passwords for GSuite so I am unable to > git-send-email it without talking to a host of corporate IT people... > I'll have to send a ... urgh... PR No worries, I don't mind GH PRs, as long as the patch looks good. The problem is that lots of GH PRs end up being done poorly. -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-21 14:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-21 0:46 Poll ring behavior broken by f0c5c54945ae92a00cdbb43bdf3abaeab6bd3a23 Glauber Costa 2020-08-21 1:57 ` Jens Axboe 2020-08-21 2:24 ` Glauber Costa 2020-08-21 3:42 ` Jens Axboe 2020-08-21 13:55 ` Glauber Costa 2020-08-21 14:36 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox