On 2020-03-13 11:46 a.m., Sebastian Andrzej Siewior wrote: > The poll callback is abusing the completion wait queue and sticks it into > poll_wait() to wake up pollers after a command has completed. > > First of all it's a layering violation as it imposes restrictions on the > inner workings of completions. Just because C allows to do so does not > justify that in any way. The proper way to do such things is to post > patches which extend the core infrastructure and not by silently abusing > it. As I've said previously, I disagree with this approach. Open coding standard primitives sweeps issues under the rug and is a step backwards for code quality. Calling it a layering violation is just one opinion and if it is, the better solution would be to create an interface you find appropriate so that it isn't one. > Aside of that the implementation is seriously broken: > > 1) It cannot work with EPOLLEXCLUSIVE Why? You don't explain this. And I don't see how this patch would change anything to do with the call to poll_wait(). All you've done is open-code the completion. Not that it matters that much, having multiple waiters poll on this interface can pretty much never happen. It only makes sense for the process who submitted the write to poll on the interface. > 2) It's racy: > > poll() write() > switchtec_dev_poll() switchtec_dev_write() > poll_wait(&s->comp.wait); mrpc_queue_cmd() > init_completion(&s->comp) > init_waitqueue_head(&s->comp.wait) That's a nice catch! But wouldn't an easier solution be to change the code to use reinit_completion() instead of using the bug to justify a different change? Thanks, Logan
