| Message ID | 1550118402-4057-10-git-send-email-wexu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | packed ring virtio-net backends support | expand |
On 2019/2/14 下午12:26, wexu@redhat.com wrote: > From: Wei Xu <wexu@redhat.com> > > This is a helper for packed ring. > > To support packed ring, the head descriptor in a chain should be updated > lastly since no 'avail_idx' like in packed ring to explicitly tell the > driver side that all payload is ready after having done the chain, so > the head is always visible immediately. > > This patch fills the header after done all the other ones. > > Signed-off-by: Wei Xu <wexu@redhat.com> It's really odd to workaround API issue in the implementation of device. Please introduce batched used updating helpers instead. Thanks > --- > hw/net/virtio-net.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 3f319ef..330abea 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -1251,6 +1251,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > struct virtio_net_hdr_mrg_rxbuf mhdr; > unsigned mhdr_cnt = 0; > size_t offset, i, guest_offset; > + VirtQueueElement head; > + int head_len = 0; > > if (!virtio_net_can_receive(nc)) { > return -1; > @@ -1328,7 +1330,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > } > > /* signal other side */ > - virtqueue_fill(q->rx_vq, elem, total, i++); > + if (i == 0) { > + head_len = total; > + head = *elem; > + } else { > + virtqueue_fill(q->rx_vq, elem, len, i); > + } > + i++; > g_free(elem); > } > > @@ -1339,6 +1347,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > &mhdr.num_buffers, sizeof mhdr.num_buffers); > } > > + virtqueue_fill(q->rx_vq, &head, head_len, 0); > virtqueue_flush(q->rx_vq, i); > virtio_notify(vdev, q->rx_vq); >
On Tue, Feb 19, 2019 at 03:23:01PM +0800, Jason Wang wrote: > > On 2019/2/14 下午12:26, wexu@redhat.com wrote: > >From: Wei Xu <wexu@redhat.com> > > > >This is a helper for packed ring. > > > >To support packed ring, the head descriptor in a chain should be updated > >lastly since no 'avail_idx' like in packed ring to explicitly tell the > >driver side that all payload is ready after having done the chain, so > >the head is always visible immediately. > > > >This patch fills the header after done all the other ones. > > > >Signed-off-by: Wei Xu <wexu@redhat.com> > > > It's really odd to workaround API issue in the implementation of device. > Please introduce batched used updating helpers instead. Can you elaborate a bit more? I don't get it as well. The exact batch as vhost-net or dpdk pmd is not supported by userspace backend. The change here is to keep the header descriptor updated at last in case of a chaining descriptors and the helper might not help too much. Wei > > Thanks > > > >--- > > hw/net/virtio-net.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > >diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >index 3f319ef..330abea 100644 > >--- a/hw/net/virtio-net.c > >+++ b/hw/net/virtio-net.c > >@@ -1251,6 +1251,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > > struct virtio_net_hdr_mrg_rxbuf mhdr; > > unsigned mhdr_cnt = 0; > > size_t offset, i, guest_offset; > >+ VirtQueueElement head; > >+ int head_len = 0; > > if (!virtio_net_can_receive(nc)) { > > return -1; > >@@ -1328,7 +1330,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > > } > > /* signal other side */ > >- virtqueue_fill(q->rx_vq, elem, total, i++); > >+ if (i == 0) { > >+ head_len = total; > >+ head = *elem; > >+ } else { > >+ virtqueue_fill(q->rx_vq, elem, len, i); > >+ } > >+ i++; > > g_free(elem); > > } > >@@ -1339,6 +1347,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > > &mhdr.num_buffers, sizeof mhdr.num_buffers); > > } > >+ virtqueue_fill(q->rx_vq, &head, head_len, 0); > > virtqueue_flush(q->rx_vq, i); > > virtio_notify(vdev, q->rx_vq); >
On 2019/2/19 下午6:51, Wei Xu wrote: > On Tue, Feb 19, 2019 at 03:23:01PM +0800, Jason Wang wrote: >> On 2019/2/14 下午12:26, wexu@redhat.com wrote: >>> From: Wei Xu <wexu@redhat.com> >>> >>> This is a helper for packed ring. >>> >>> To support packed ring, the head descriptor in a chain should be updated >>> lastly since no 'avail_idx' like in packed ring to explicitly tell the >>> driver side that all payload is ready after having done the chain, so >>> the head is always visible immediately. >>> >>> This patch fills the header after done all the other ones. >>> >>> Signed-off-by: Wei Xu <wexu@redhat.com> >> >> It's really odd to workaround API issue in the implementation of device. >> Please introduce batched used updating helpers instead. > Can you elaborate a bit more? I don't get it as well. > > The exact batch as vhost-net or dpdk pmd is not supported by userspace > backend. The change here is to keep the header descriptor updated at > last in case of a chaining descriptors and the helper might not help > too much. > > Wei Of course we can add batching support why not? Your code assumes the device know the virtio layout specific assumption which breaks the layer. Device should not care about the actual layout. Thanks >> Thanks >> >> >>> --- >>> hw/net/virtio-net.c | 11 ++++++++++- >>> 1 file changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>> index 3f319ef..330abea 100644 >>> --- a/hw/net/virtio-net.c >>> +++ b/hw/net/virtio-net.c >>> @@ -1251,6 +1251,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, >>> struct virtio_net_hdr_mrg_rxbuf mhdr; >>> unsigned mhdr_cnt = 0; >>> size_t offset, i, guest_offset; >>> + VirtQueueElement head; >>> + int head_len = 0; >>> if (!virtio_net_can_receive(nc)) { >>> return -1; >>> @@ -1328,7 +1330,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, >>> } >>> /* signal other side */ >>> - virtqueue_fill(q->rx_vq, elem, total, i++); >>> + if (i == 0) { >>> + head_len = total; >>> + head = *elem; >>> + } else { >>> + virtqueue_fill(q->rx_vq, elem, len, i); >>> + } >>> + i++; >>> g_free(elem); >>> } >>> @@ -1339,6 +1347,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, >>> &mhdr.num_buffers, sizeof mhdr.num_buffers); >>> } >>> + virtqueue_fill(q->rx_vq, &head, head_len, 0); >>> virtqueue_flush(q->rx_vq, i); >>> virtio_notify(vdev, q->rx_vq);
On Tue, Feb 19, 2019 at 09:09:33PM +0800, Jason Wang wrote: > > On 2019/2/19 下午6:51, Wei Xu wrote: > >On Tue, Feb 19, 2019 at 03:23:01PM +0800, Jason Wang wrote: > >>On 2019/2/14 下午12:26, wexu@redhat.com wrote: > >>>From: Wei Xu <wexu@redhat.com> > >>> > >>>This is a helper for packed ring. > >>> > >>>To support packed ring, the head descriptor in a chain should be updated > >>>lastly since no 'avail_idx' like in packed ring to explicitly tell the > >>>driver side that all payload is ready after having done the chain, so > >>>the head is always visible immediately. > >>> > >>>This patch fills the header after done all the other ones. > >>> > >>>Signed-off-by: Wei Xu <wexu@redhat.com> > >> > >>It's really odd to workaround API issue in the implementation of device. > >>Please introduce batched used updating helpers instead. > >Can you elaborate a bit more? I don't get it as well. > > > >The exact batch as vhost-net or dpdk pmd is not supported by userspace > >backend. The change here is to keep the header descriptor updated at > >last in case of a chaining descriptors and the helper might not help > >too much. > > > >Wei > > > Of course we can add batching support why not? It is always good to improve performance with anything, while probably this could be done in another separate batch, also we need to bear in mind that usually qemu userspace backend is not the first option for performance oriented user. AFAICT, virtqueue_fill() is a generic API for all relevant userspace virtio devices that do not support batching , without touching virtqueue_fill(), supporting batching changes the meaning of the parameter 'idx' which should be kept overall. To fix it, I got two proposals so far: 1). batching support(two APIs needed to keep compatibility) 2). save a head elem for a vq instead of caching an array of elems like vhost, and introduce a new API(virtqueue_chain_fill()) functioning with an additional parameter 'more' to the current virtqueue_fill() to indicate if there are more descriptor(s) coming in a chain. Either way it changes the API somehow and it does not seem to be clean and clear as wanted. Any better idea? > > Your code assumes the device know the virtio layout specific assumption > whih breaks the layer. Device should not care about the actual layout. > Good point, but anyway, change to virtio-net receiving code path is unavoidable to support split and packed rings, and batching is like a new feature somehow. Wei > Thanks > > > >>Thanks > >> > >> > >>>--- > >>> hw/net/virtio-net.c | 11 ++++++++++- > >>> 1 file changed, 10 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>>index 3f319ef..330abea 100644 > >>>--- a/hw/net/virtio-net.c > >>>+++ b/hw/net/virtio-net.c > >>>@@ -1251,6 +1251,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > >>> struct virtio_net_hdr_mrg_rxbuf mhdr; > >>> unsigned mhdr_cnt = 0; > >>> size_t offset, i, guest_offset; > >>>+ VirtQueueElement head; > >>>+ int head_len = 0; > >>> if (!virtio_net_can_receive(nc)) { > >>> return -1; > >>>@@ -1328,7 +1330,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > >>> } > >>> /* signal other side */ > >>>- virtqueue_fill(q->rx_vq, elem, total, i++); > >>>+ if (i == 0) { > >>>+ head_len = total; > >>>+ head = *elem; > >>>+ } else { > >>>+ virtqueue_fill(q->rx_vq, elem, len, i); > >>>+ } > >>>+ i++; > >>> g_free(elem); > >>> } > >>>@@ -1339,6 +1347,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > >>> &mhdr.num_buffers, sizeof mhdr.num_buffers); > >>> } > >>>+ virtqueue_fill(q->rx_vq, &head, head_len, 0); > >>> virtqueue_flush(q->rx_vq, i); > >>> virtio_notify(vdev, q->rx_vq); >
On 2019/2/20 上午9:54, Wei Xu wrote: > On Tue, Feb 19, 2019 at 09:09:33PM +0800, Jason Wang wrote: >> On 2019/2/19 下午6:51, Wei Xu wrote: >>> On Tue, Feb 19, 2019 at 03:23:01PM +0800, Jason Wang wrote: >>>> On 2019/2/14 下午12:26, wexu@redhat.com wrote: >>>>> From: Wei Xu <wexu@redhat.com> >>>>> >>>>> This is a helper for packed ring. >>>>> >>>>> To support packed ring, the head descriptor in a chain should be updated >>>>> lastly since no 'avail_idx' like in packed ring to explicitly tell the >>>>> driver side that all payload is ready after having done the chain, so >>>>> the head is always visible immediately. >>>>> >>>>> This patch fills the header after done all the other ones. >>>>> >>>>> Signed-off-by: Wei Xu <wexu@redhat.com> >>>> It's really odd to workaround API issue in the implementation of device. >>>> Please introduce batched used updating helpers instead. >>> Can you elaborate a bit more? I don't get it as well. >>> >>> The exact batch as vhost-net or dpdk pmd is not supported by userspace >>> backend. The change here is to keep the header descriptor updated at >>> last in case of a chaining descriptors and the helper might not help >>> too much. >>> >>> Wei >> >> Of course we can add batching support why not? > It is always good to improve performance with anything, while probably > this could be done in another separate batch, also we need to bear > in mind that usually qemu userspace backend is not the first option for > performance oriented user. The point is to hide layout specific things from device emulation. If it helps for performance, it could be treated as a good byproduct. > > AFAICT, virtqueue_fill() is a generic API for all relevant userspace virtio > devices that do not support batching , without touching virtqueue_fill(), > supporting batching changes the meaning of the parameter 'idx' which should > be kept overall. > > To fix it, I got two proposals so far: > 1). batching support(two APIs needed to keep compatibility) > 2). save a head elem for a vq instead of caching an array of elems like vhost, > and introduce a new API(virtqueue_chain_fill()) functioning with an > additional parameter 'more' to the current virtqueue_fill() to indicate if > there are more descriptor(s) coming in a chain. > > Either way it changes the API somehow and it does not seem to be clean and clear > as wanted. It's as simple as accepting an array of elems in e.g virtqueue_fill_batched()? > > Any better idea? > >> Your code assumes the device know the virtio layout specific assumption >> whih breaks the layer. Device should not care about the actual layout. >> > Good point, but anyway, change to virtio-net receiving code path is > unavoidable to support split and packed rings, and batching is like a new > feature somehow. It's ok to change the code as a result of introducing of generic helper but it's bad to change to code for working around a bad API. Thanks > > Wei > >> Thanks >> >> >>>> Thanks >>>> >>>> >>>>> --- >>>>> hw/net/virtio-net.c | 11 ++++++++++- >>>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>>> index 3f319ef..330abea 100644 >>>>> --- a/hw/net/virtio-net.c >>>>> +++ b/hw/net/virtio-net.c >>>>> @@ -1251,6 +1251,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, >>>>> struct virtio_net_hdr_mrg_rxbuf mhdr; >>>>> unsigned mhdr_cnt = 0; >>>>> size_t offset, i, guest_offset; >>>>> + VirtQueueElement head; >>>>> + int head_len = 0; >>>>> if (!virtio_net_can_receive(nc)) { >>>>> return -1; >>>>> @@ -1328,7 +1330,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, >>>>> } >>>>> /* signal other side */ >>>>> - virtqueue_fill(q->rx_vq, elem, total, i++); >>>>> + if (i == 0) { >>>>> + head_len = total; >>>>> + head = *elem; >>>>> + } else { >>>>> + virtqueue_fill(q->rx_vq, elem, len, i); >>>>> + } >>>>> + i++; >>>>> g_free(elem); >>>>> } >>>>> @@ -1339,6 +1347,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, >>>>> &mhdr.num_buffers, sizeof mhdr.num_buffers); >>>>> } >>>>> + virtqueue_fill(q->rx_vq, &head, head_len, 0); >>>>> virtqueue_flush(q->rx_vq, i); >>>>> virtio_notify(vdev, q->rx_vq);
On Wed, Feb 20, 2019 at 10:34:32AM +0800, Jason Wang wrote: > > On 2019/2/20 上午9:54, Wei Xu wrote: > >On Tue, Feb 19, 2019 at 09:09:33PM +0800, Jason Wang wrote: > >>On 2019/2/19 下午6:51, Wei Xu wrote: > >>>On Tue, Feb 19, 2019 at 03:23:01PM +0800, Jason Wang wrote: > >>>>On 2019/2/14 下午12:26, wexu@redhat.com wrote: > >>>>>From: Wei Xu <wexu@redhat.com> > >>>>> > >>>>>This is a helper for packed ring. > >>>>> > >>>>>To support packed ring, the head descriptor in a chain should be updated > >>>>>lastly since no 'avail_idx' like in packed ring to explicitly tell the > >>>>>driver side that all payload is ready after having done the chain, so > >>>>>the head is always visible immediately. > >>>>> > >>>>>This patch fills the header after done all the other ones. > >>>>> > >>>>>Signed-off-by: Wei Xu <wexu@redhat.com> > >>>>It's really odd to workaround API issue in the implementation of device. > >>>>Please introduce batched used updating helpers instead. > >>>Can you elaborate a bit more? I don't get it as well. > >>> > >>>The exact batch as vhost-net or dpdk pmd is not supported by userspace > >>>backend. The change here is to keep the header descriptor updated at > >>>last in case of a chaining descriptors and the helper might not help > >>>too much. > >>> > >>>Wei > >> > >>Of course we can add batching support why not? > >It is always good to improve performance with anything, while probably > >this could be done in another separate batch, also we need to bear > >in mind that usually qemu userspace backend is not the first option for > >performance oriented user. > > > The point is to hide layout specific things from device emulation. If it > helps for performance, it could be treated as a good byproduct. > > > > > >AFAICT, virtqueue_fill() is a generic API for all relevant userspace virtio > >devices that do not support batching , without touching virtqueue_fill(), > >supporting batching changes the meaning of the parameter 'idx' which should > >be kept overall. > > > >To fix it, I got two proposals so far: > >1). batching support(two APIs needed to keep compatibility) > >2). save a head elem for a vq instead of caching an array of elems like vhost, > > and introduce a new API(virtqueue_chain_fill()) functioning with an > > additional parameter 'more' to the current virtqueue_fill() to indicate if > > there are more descriptor(s) coming in a chain. > > > >Either way it changes the API somehow and it does not seem to be clean and clear > >as wanted. > > > It's as simple as accepting an array of elems in e.g > virtqueue_fill_batched()? It is trivial for both, my concern is an array elements need to be allocated dynamically due to vq size which no any other devices are using, a head would be enough for 2. > > > > > >Any better idea? > > > >>Your code assumes the device know the virtio layout specific assumption > >>whih breaks the layer. Device should not care about the actual layout. > >> > >Good point, but anyway, change to virtio-net receiving code path is > >unavoidable to support split and packed rings, and batching is like a new > >feature somehow. > > > It's ok to change the code as a result of introducing of generic helper but > it's bad to change to code for working around a bad API. Agree. Wei > > Thanks > > > > > >Wei > >>Thanks > >> > >> > >>>>Thanks > >>>> > >>>> > >>>>>--- > >>>>> hw/net/virtio-net.c | 11 ++++++++++- > >>>>> 1 file changed, 10 insertions(+), 1 deletion(-) > >>>>> > >>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>>>>index 3f319ef..330abea 100644 > >>>>>--- a/hw/net/virtio-net.c > >>>>>+++ b/hw/net/virtio-net.c > >>>>>@@ -1251,6 +1251,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > >>>>> struct virtio_net_hdr_mrg_rxbuf mhdr; > >>>>> unsigned mhdr_cnt = 0; > >>>>> size_t offset, i, guest_offset; > >>>>>+ VirtQueueElement head; > >>>>>+ int head_len = 0; > >>>>> if (!virtio_net_can_receive(nc)) { > >>>>> return -1; > >>>>>@@ -1328,7 +1330,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > >>>>> } > >>>>> /* signal other side */ > >>>>>- virtqueue_fill(q->rx_vq, elem, total, i++); > >>>>>+ if (i == 0) { > >>>>>+ head_len = total; > >>>>>+ head = *elem; > >>>>>+ } else { > >>>>>+ virtqueue_fill(q->rx_vq, elem, len, i); > >>>>>+ } > >>>>>+ i++; > >>>>> g_free(elem); > >>>>> } > >>>>>@@ -1339,6 +1347,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > >>>>> &mhdr.num_buffers, sizeof mhdr.num_buffers); > >>>>> } > >>>>>+ virtqueue_fill(q->rx_vq, &head, head_len, 0); > >>>>> virtqueue_flush(q->rx_vq, i); > >>>>> virtio_notify(vdev, q->rx_vq); >
On 2019/2/20 下午12:01, Wei Xu wrote: >> >>> AFAICT, virtqueue_fill() is a generic API for all relevant userspace virtio >>> devices that do not support batching , without touching virtqueue_fill(), >>> supporting batching changes the meaning of the parameter 'idx' which should >>> be kept overall. >>> >>> To fix it, I got two proposals so far: >>> 1). batching support(two APIs needed to keep compatibility) >>> 2). save a head elem for a vq instead of caching an array of elems like vhost, >>> and introduce a new API(virtqueue_chain_fill()) functioning with an >>> additional parameter 'more' to the current virtqueue_fill() to indicate if >>> there are more descriptor(s) coming in a chain. >>> >>> Either way it changes the API somehow and it does not seem to be clean and clear >>> as wanted. >> It's as simple as accepting an array of elems in e.g >> virtqueue_fill_batched()? > It is trivial for both, my concern is an array elements need to be allocated dynamically > due to vq size which no any other devices are using, a head would be enough for 2. > Do you see any issue for this? Thanks
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 3f319ef..330abea 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1251,6 +1251,8 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, struct virtio_net_hdr_mrg_rxbuf mhdr; unsigned mhdr_cnt = 0; size_t offset, i, guest_offset; + VirtQueueElement head; + int head_len = 0; if (!virtio_net_can_receive(nc)) { return -1; @@ -1328,7 +1330,13 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, } /* signal other side */ - virtqueue_fill(q->rx_vq, elem, total, i++); + if (i == 0) { + head_len = total; + head = *elem; + } else { + virtqueue_fill(q->rx_vq, elem, len, i); + } + i++; g_free(elem); } @@ -1339,6 +1347,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, &mhdr.num_buffers, sizeof mhdr.num_buffers); } + virtqueue_fill(q->rx_vq, &head, head_len, 0); virtqueue_flush(q->rx_vq, i); virtio_notify(vdev, q->rx_vq);