Jason Wang
2021-Jul-14 06:02 UTC
[PATCH v9 16/17] vduse: Introduce VDUSE - vDPA Device in Userspace
? 2021/7/14 ??1:54, Michael S. Tsirkin ??:> On Wed, Jul 14, 2021 at 01:45:39PM +0800, Jason Wang wrote: >>> +static int vduse_dev_msg_sync(struct vduse_dev *dev, >>> + struct vduse_dev_msg *msg) >>> +{ >>> + int ret; >>> + >>> + init_waitqueue_head(&msg->waitq); >>> + spin_lock(&dev->msg_lock); >>> + msg->req.request_id = dev->msg_unique++; >>> + vduse_enqueue_msg(&dev->send_list, msg); >>> + wake_up(&dev->waitq); >>> + spin_unlock(&dev->msg_lock); >>> + >>> + wait_event_killable_timeout(msg->waitq, msg->completed, >>> + VDUSE_REQUEST_TIMEOUT * HZ); >>> + spin_lock(&dev->msg_lock); >>> + if (!msg->completed) { >>> + list_del(&msg->list); >>> + msg->resp.result = VDUSE_REQ_RESULT_FAILED; >>> + } >>> + ret = (msg->resp.result == VDUSE_REQ_RESULT_OK) ? 0 : -EIO; >> >> I think we should mark the device as malfunction when there is a timeout and >> forbid any userspace operations except for the destroy aftwards for safety. > This looks like if one tried to run gdb on the program the behaviour > will change completely because kernel wants it to respond within > specific time. Looks like a receipe for heisenbugs. > > Let's not build interfaces with arbitrary timeouts like that. > Interruptible wait exists for this very reason.The problem is. Do we want userspace program like modprobe to be stuck for indefinite time and expect the administrator to kill that? Thanks> Let userspace have its > own code to set and use timers. This does mean that userspace will > likely have to change a bit to support this driver, such is life. >
Greg KH
2021-Jul-14 06:47 UTC
[PATCH v9 16/17] vduse: Introduce VDUSE - vDPA Device in Userspace
On Wed, Jul 14, 2021 at 02:02:50PM +0800, Jason Wang wrote:> > ? 2021/7/14 ??1:54, Michael S. Tsirkin ??: > > On Wed, Jul 14, 2021 at 01:45:39PM +0800, Jason Wang wrote: > > > > +static int vduse_dev_msg_sync(struct vduse_dev *dev, > > > > + struct vduse_dev_msg *msg) > > > > +{ > > > > + int ret; > > > > + > > > > + init_waitqueue_head(&msg->waitq); > > > > + spin_lock(&dev->msg_lock); > > > > + msg->req.request_id = dev->msg_unique++; > > > > + vduse_enqueue_msg(&dev->send_list, msg); > > > > + wake_up(&dev->waitq); > > > > + spin_unlock(&dev->msg_lock); > > > > + > > > > + wait_event_killable_timeout(msg->waitq, msg->completed, > > > > + VDUSE_REQUEST_TIMEOUT * HZ); > > > > + spin_lock(&dev->msg_lock); > > > > + if (!msg->completed) { > > > > + list_del(&msg->list); > > > > + msg->resp.result = VDUSE_REQ_RESULT_FAILED; > > > > + } > > > > + ret = (msg->resp.result == VDUSE_REQ_RESULT_OK) ? 0 : -EIO; > > > > > > I think we should mark the device as malfunction when there is a timeout and > > > forbid any userspace operations except for the destroy aftwards for safety. > > This looks like if one tried to run gdb on the program the behaviour > > will change completely because kernel wants it to respond within > > specific time. Looks like a receipe for heisenbugs. > > > > Let's not build interfaces with arbitrary timeouts like that. > > Interruptible wait exists for this very reason. > > > The problem is. Do we want userspace program like modprobe to be stuck for > indefinite time and expect the administrator to kill that?Why would modprobe be stuck for forever? Is this on the module probe path?