search for: add_outbuf

...ead ends I implemented the simplest solution: the struct virtio_device contains the spinlock to use, and it's held when your callbacks get called. Core changes: 1) struct virtio_device has a "lock" and "priv" fields (the latter for the driver to use, esp from callbacks). 2) add_outbuf and add_inbuf take a cb function ptr and void *, rather than a used pointer. 3) lengths of buffers must now fit in an unsigned int, not long. 4) the virtio_sync wrapper is gone. Block driver changes (+30 lines) 1) Convert to callbacks not interrupt. 2) Ensure both outbuf & inbuf have been used...

...ead ends I implemented the simplest solution: the struct virtio_device contains the spinlock to use, and it's held when your callbacks get called. Core changes: 1) struct virtio_device has a "lock" and "priv" fields (the latter for the driver to use, esp from callbacks). 2) add_outbuf and add_inbuf take a cb function ptr and void *, rather than a used pointer. 3) lengths of buffers must now fit in an unsigned int, not long. 4) the virtio_sync wrapper is gone. Block driver changes (+30 lines) 1) Convert to callbacks not interrupt. 2) Ensure both outbuf & inbuf have been used...

...ead ends I implemented the simplest solution: the struct virtio_device contains the spinlock to use, and it's held when your callbacks get called. Core changes: 1) struct virtio_device has a "lock" and "priv" fields (the latter for the driver to use, esp from callbacks). 2) add_outbuf and add_inbuf take a cb function ptr and void *, rather than a used pointer. 3) lengths of buffers must now fit in an unsigned int, not long. 4) the virtio_sync wrapper is gone. Block driver changes (+30 lines) 1) Convert to callbacks not interrupt. 2) Ensure both outbuf & inbuf have been used...

...** + * virtio_device - description and routines to drive a virtual device. + * @dev: the underlying struct device. + * @ops: the operations for this virtual device. + */ +struct virtio_device { + struct device *dev; + struct virtio_ops *ops; +}; + +/** + * virtio_ops - virtio abstraction layer + * @add_outbuf: prepare to send data to the other end: + * vdev: the virtio_device + * sg: the description of the buffer(s). + * num: the size of the sg array. + * used: the length sent (set once sending is done). + * Returns an identifier or an error. + * @add_inbuf: prepare to receive data from the other e...

...** + * virtio_device - description and routines to drive a virtual device. + * @dev: the underlying struct device. + * @ops: the operations for this virtual device. + */ +struct virtio_device { + struct device *dev; + struct virtio_ops *ops; +}; + +/** + * virtio_ops - virtio abstraction layer + * @add_outbuf: prepare to send data to the other end: + * vdev: the virtio_device + * sg: the description of the buffer(s). + * num: the size of the sg array. + * used: the length sent (set once sending is done). + * Returns an identifier or an error. + * @add_inbuf: prepare to receive data from the other e...

...** + * virtio_device - description and routines to drive a virtual device. + * @dev: the underlying struct device. + * @ops: the operations for this virtual device. + */ +struct virtio_device { + struct device *dev; + struct virtio_ops *ops; +}; + +/** + * virtio_ops - virtio abstraction layer + * @add_outbuf: prepare to send data to the other end: + * vdev: the virtio_device + * sg: the description of the buffer(s). + * num: the size of the sg array. + * used: the length sent (set once sending is done). + * Returns an identifier or an error. + * @add_inbuf: prepare to receive data from the other e...

...alloon_oom_notify(). > > Ohh, I missed your point and thought the dependency is indirect I do think this is the case. See below. > and some > other call path is allocating while holding the lock. But you seem to be > right and > leak_balloon > tell_host > virtqueue_add_outbuf > virtqueue_add > > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should > try to allocate while we are in the OOM path. Michael, is there any way > to drop this? Yes - in practice it won't ever allocate - that path is never taken with add_outbuf - it is...

...alloon_oom_notify(). > > Ohh, I missed your point and thought the dependency is indirect I do think this is the case. See below. > and some > other call path is allocating while holding the lock. But you seem to be > right and > leak_balloon > tell_host > virtqueue_add_outbuf > virtqueue_add > > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should > try to allocate while we are in the OOM path. Michael, is there any way > to drop this? Yes - in practice it won't ever allocate - that path is never taken with add_outbuf - it is...

...> On Mon, Oct 02, 2017 at 01:50:35PM +0200, Michal Hocko wrote: > [...] > > > and some > > > other call path is allocating while holding the lock. But you seem to be > > > right and > > > leak_balloon > > > tell_host > > > virtqueue_add_outbuf > > > virtqueue_add > > > > > > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should > > > try to allocate while we are in the OOM path. Michael, is there any way > > > to drop this? > > > > Yes - in practice it won...

...> On Mon, Oct 02, 2017 at 01:50:35PM +0200, Michal Hocko wrote: > [...] > > > and some > > > other call path is allocating while holding the lock. But you seem to be > > > right and > > > leak_balloon > > > tell_host > > > virtqueue_add_outbuf > > > virtqueue_add > > > > > > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should > > > try to allocate while we are in the OOM path. Michael, is there any way > > > to drop this? > > > > Yes - in practice it won...

This is a subject that came up in the virtio BOF session at OLS. I decided to go forward and implement something that I like, based on the latest virtio proposal at the time, which was draft III. It's not a drop-in replacement, because it's missing a host implementation. I first started my own, which is not done yet, but wanted to do one for lguest and one for emulated PCI next. It's

This is a subject that came up in the virtio BOF session at OLS. I decided to go forward and implement something that I like, based on the latest virtio proposal at the time, which was draft III. It's not a drop-in replacement, because it's missing a host implementation. I first started my own, which is not done yet, but wanted to do one for lguest and one for emulated PCI next. It's

...add_one_chunk(vb, vq, > + PAGE_CHUNK_TYPE_UNUSED, > + pfn, > + (u64)(1 << order)); > + } > + } while (!ret); > + } > + } > + } > + miscq_out_hdr->flags |= VIRTIO_BALLOON_MISCQ_F_COMPLETE; And where is miscq_out_hdr used? I see no add_outbuf anywhere. Things like this should be passed through function parameters and not stuffed into device structure, fields should be initialized before use and not where we happen to have the data handy. Also, _F_ is normally a bit number, you use it as a value here. > + send_page_chunks(vb, vq...

...add_one_chunk(vb, vq, > + PAGE_CHUNK_TYPE_UNUSED, > + pfn, > + (u64)(1 << order)); > + } > + } while (!ret); > + } > + } > + } > + miscq_out_hdr->flags |= VIRTIO_BALLOON_MISCQ_F_COMPLETE; And where is miscq_out_hdr used? I see no add_outbuf anywhere. Things like this should be passed through function parameters and not stuffed into device structure, fields should be initialized before use and not where we happen to have the data handy. Also, _F_ is normally a bit number, you use it as a value here. > + send_page_chunks(vb, vq...

...PAGE_CHUNK_TYPE_UNUSED, >> + pfn, >> + (u64)(1 << order)); >> + } >> + } while (!ret); >> + } >> + } >> + } >> + miscq_out_hdr->flags |= VIRTIO_BALLOON_MISCQ_F_COMPLETE; > And where is miscq_out_hdr used? I see no add_outbuf anywhere. > > Things like this should be passed through function parameters > and not stuffed into device structure, fields should be > initialized before use and not where we happen to > have the data handy. > miscq_out_hdr is linear with the payload (i.e. kmalloc(hdr+payload) )...

...17:11:55, Michael S. Tsirkin wrote: > On Mon, Oct 02, 2017 at 01:50:35PM +0200, Michal Hocko wrote: [...] > > and some > > other call path is allocating while holding the lock. But you seem to be > > right and > > leak_balloon > > tell_host > > virtqueue_add_outbuf > > virtqueue_add > > > > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should > > try to allocate while we are in the OOM path. Michael, is there any way > > to drop this? > > Yes - in practice it won't ever allocate - that path is n...

.... > > > > > > > > + > > > + /* Enqueue the request */ > > > + dev_dbg(&vq->vdev->dev, "%s\n", __func__); > > > + ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC); > > > > > > This is easier as add_outbuf. > > Will look into it. > > > > > Also - why GFP_ATOMIC? > > Hmm..., may be it can be GFP_KERNEL. I don't see atomic context here. Will > look into it. > > > > > > + if (ret < 0) { > > > + if (ret == -ENOMEM || ret == -ENOSPC)...

...n, > > > + (u64)(1 << order)); > > > + } > > > + } while (!ret); > > > + } > > > + } > > > + } > > > + miscq_out_hdr->flags |= VIRTIO_BALLOON_MISCQ_F_COMPLETE; > > And where is miscq_out_hdr used? I see no add_outbuf anywhere. > > > > Things like this should be passed through function parameters > > and not stuffed into device structure, fields should be > > initialized before use and not where we happen to > > have the data handy. > > > > miscq_out_hdr is linear wit...

...n, > > > + (u64)(1 << order)); > > > + } > > > + } while (!ret); > > > + } > > > + } > > > + } > > > + miscq_out_hdr->flags |= VIRTIO_BALLOON_MISCQ_F_COMPLETE; > > And where is miscq_out_hdr used? I see no add_outbuf anywhere. > > > > Things like this should be passed through function parameters > > and not stuffed into device structure, fields should be > > initialized before use and not where we happen to > > have the data handy. > > > > miscq_out_hdr is linear wit...

...eded for guest. Will look into getting rid of this. > > > > + > > + /* Enqueue the request */ > > + dev_dbg(&vq->vdev->dev, "%s\n", __func__); > > + ret = virtqueue_add_sgs(vq, sgs, 1, 0, forget, GFP_ATOMIC); > > > This is easier as add_outbuf. Will look into it. > > Also - why GFP_ATOMIC? Hmm..., may be it can be GFP_KERNEL. I don't see atomic context here. Will look into it. > > > + if (ret < 0) { > > + if (ret == -ENOMEM || ret == -ENOSPC) { > > + pr_debug("virtio-fs: Could not queue...