lkml.org 
[lkml]   [2018]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC v4 3/5] virtio_ring: add packed ring support
    From
    Date


    On 2018年05月16日 21:45, Tiwei Bie wrote:
    > On Wed, May 16, 2018 at 08:51:43PM +0800, Jason Wang wrote:
    >> On 2018年05月16日 20:39, Tiwei Bie wrote:
    >>> On Wed, May 16, 2018 at 07:50:16PM +0800, Jason Wang wrote:
    >>>> On 2018年05月16日 16:37, Tiwei Bie wrote:
    >>> [...]
    >>>>> struct vring_virtqueue {
    >>>>> @@ -116,6 +117,9 @@ struct vring_virtqueue {
    >>>>> /* Last written value to driver->flags in
    >>>>> * guest byte order. */
    >>>>> u16 event_flags_shadow;
    >>>>> +
    >>>>> + /* ID allocation. */
    >>>>> + struct idr buffer_id;
    >>>> I'm not sure idr is fit for the performance critical case here. Need to
    >>>> measure its performance impact, especially if we have few unused slots.
    >>> I'm also not sure.. But fortunately, it should be quite easy
    >>> to replace it with something else without changing other code.
    >>> If it will really hurt the performance, I'll change it.
    >> We may want to do some benchmarking/profiling to see.
    > Yeah!
    >
    >>>>> };
    >>>>> };
    >>> [...]
    >>>>> +static void detach_buf_packed(struct vring_virtqueue *vq, unsigned int head,
    >>>>> + unsigned int id, void **ctx)
    >>>>> +{
    >>>>> + struct vring_packed_desc *desc;
    >>>>> + unsigned int i, j;
    >>>>> +
    >>>>> + /* Clear data ptr. */
    >>>>> + vq->desc_state[id].data = NULL;
    >>>>> +
    >>>>> + i = head;
    >>>>> +
    >>>>> + for (j = 0; j < vq->desc_state[id].num; j++) {
    >>>>> + desc = &vq->vring_packed.desc[i];
    >>>>> + vring_unmap_one_packed(vq, desc);
    >>>> As mentioned in previous discussion, this probably won't work for the case
    >>>> of out of order completion since it depends on the information in the
    >>>> descriptor ring. We probably need to extend ctx to record such information.
    >>> Above code doesn't depend on the information in the descriptor
    >>> ring. The vq->desc_state[] is the extended ctx.
    >>>
    >>> Best regards,
    >>> Tiwei Bie
    >> Yes, but desc is a pointer to descriptor ring I think so
    >> vring_unmap_one_packed() still depends on the content of descriptor ring?
    >>
    > I got your point now. I think it makes sense to reserve
    > the bits of the addr field. Driver shouldn't try to get
    > addrs from the descriptors when cleanup the descriptors
    > no matter whether we support out-of-order or not.

    Maybe I was wrong, but I remember spec mentioned something like this.

    >
    > But combining it with the out-of-order support, it will
    > mean that the driver still needs to maintain a desc/ctx
    > list that is very similar to the desc ring in the split
    > ring. I'm not quite sure whether it's something we want.
    > If it is true, I'll do it. So do you think we also want
    > to maintain such a desc/ctx list for packed ring?

    To make it work for OOO backends I think we need something like this
    (hardware NIC drivers are usually have something like this).

    Not for the patch, but it looks like having a OUT_OF_ORDER feature bit
    is much more simpler to be started with.

    Thanks

    >
    > Best regards,
    > Tiwei Bie

    \
     
     \ /
      Last update: 2018-05-16 16:07    [W:3.657 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site