[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1726126234.0404134-1-xuanzhuo@linux.alibaba.com>
Date: Thu, 12 Sep 2024 15:30:34 +0800
From: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
To: Jason Wang <jasowang@...hat.com>
Cc: netdev@...r.kernel.org,
"Michael S. Tsirkin" <mst@...hat.com>,
Eugenio Pérez <eperezma@...hat.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Jesper Dangaard Brouer <hawk@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
virtualization@...ts.linux.dev,
bpf@...r.kernel.org
Subject: Re: [PATCH net-next 02/13] virtio_ring: split: harden dma unmap for indirect
On Wed, 11 Sep 2024 11:46:30 +0800, Jason Wang <jasowang@...hat.com> wrote:
> On Tue, Aug 20, 2024 at 3:33 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> >
> > 1. this commit hardens dma unmap for indirect
>
> I think we need to explain why we need such hardening. For example
> indirect use stream mapping which is read-only from the device. So it
> looks to me like it doesn't require hardening by itself.
>
> > 2. the subsequent commit uses the struct extra to record whether the
> > buffers need to be unmapped or not.
>
> It's better to explain why such a decision could not be implied with
> the existing metadata.
>
> > So we need a struct extra for
> > every desc, whatever it is indirect or not.
>
> If this is the real reason, we need to tweak the title.
YES. It is.
I will tweak the title in next version.
>
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > ---
> > drivers/virtio/virtio_ring.c | 122 ++++++++++++++++-------------------
> > 1 file changed, 57 insertions(+), 65 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 228e9fbcba3f..582d2c05498a 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -67,9 +67,16 @@
> > #define LAST_ADD_TIME_INVALID(vq)
> > #endif
> >
> > +struct vring_desc_extra {
> > + dma_addr_t addr; /* Descriptor DMA addr. */
> > + u32 len; /* Descriptor length. */
> > + u16 flags; /* Descriptor flags. */
> > + u16 next; /* The next desc state in a list. */
> > +};
> > +
> > struct vring_desc_state_split {
> > void *data; /* Data for callback. */
> > - struct vring_desc *indir_desc; /* Indirect descriptor, if any. */
> > + struct vring_desc_extra *indir; /* Indirect descriptor, if any. */
>
> Btw, it might be worth explaining that this will be allocated with an
> indirect descriptor table so we won't stress more to the memory
> allocator.
Will do.
Thanks.
>
> Thanks
>
>
Powered by blists - more mailing lists