Xuan Zhuo
2023-Aug-16 02:08 UTC
[PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()
On Wed, 16 Aug 2023 09:13:48 +0800, Jason Wang <jasowang at redhat.com> wrote:> On Tue, Aug 15, 2023 at 5:40?PM Xuan Zhuo <xuanzhuo at linux.alibaba.com> wrote: > > > > On Tue, 15 Aug 2023 15:50:23 +0800, Jason Wang <jasowang at redhat.com> wrote: > > > On Tue, Aug 15, 2023 at 2:32?PM Xuan Zhuo <xuanzhuo at linux.alibaba.com> wrote: > > > > > > > > > > > > Hi, Jason > > > > > > > > Could you skip this patch? > > > > > > I'm fine with either merging or dropping this. > > > > > > > > > > > Let we review other patches firstly? > > > > > > I will be on vacation soon, and won't have time to do this until next week. > > > > Have a happly vacation. > > > > > > > > But I spot two possible "issues": > > > > > > 1) the DMA metadata were stored in the headroom of the page, this > > > breaks frags coalescing, we need to benchmark it's impact > > > > Not every page, just the first page of the COMP pages. > > > > So I think there is no impact. > > Nope, see this: > > if (SKB_FRAG_PAGE_ORDER && > !static_branch_unlikely(&net_high_order_alloc_disable_key)) { > /* Avoid direct reclaim but allow kswapd to wake */ > pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) | > __GFP_COMP | __GFP_NOWARN | > __GFP_NORETRY, > SKB_FRAG_PAGE_ORDER); > if (likely(pfrag->page)) { > pfrag->size = PAGE_SIZE << SKB_FRAG_PAGE_ORDER; > return true; > } > } > > The comp page might be disabled due to the SKB_FRAG_PAGE_ORDER and > net_high_order_alloc_disable_key.YES. But if comp page is disabled. Then we only get one page each time. The pages are not contiguous, so we don't have frags coalescing. If you mean the two pages got from alloc_page may be contiguous. The coalescing may then be broken. It's a possibility, but I think the impact will be small. Thanks.> > > > > > > > 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT > > > > Because that the tx is not the premapped mode. > > Yes, we can optimize this on top. > > Thanks > > > > > Thanks. > > > > > > > > I see Michael has merge this series so I'm fine to let it go first. > > > > > > Thanks > > > > > > > > > > > Thanks. > > > > > > > > > >
Jason Wang
2023-Aug-16 02:19 UTC
[PATCH vhost v13 05/12] virtio_ring: introduce virtqueue_dma_dev()
On Wed, Aug 16, 2023 at 10:16?AM Xuan Zhuo <xuanzhuo at linux.alibaba.com> wrote:> > On Wed, 16 Aug 2023 09:13:48 +0800, Jason Wang <jasowang at redhat.com> wrote: > > On Tue, Aug 15, 2023 at 5:40?PM Xuan Zhuo <xuanzhuo at linux.alibaba.com> wrote: > > > > > > On Tue, 15 Aug 2023 15:50:23 +0800, Jason Wang <jasowang at redhat.com> wrote: > > > > On Tue, Aug 15, 2023 at 2:32?PM Xuan Zhuo <xuanzhuo at linux.alibaba.com> wrote: > > > > > > > > > > > > > > > Hi, Jason > > > > > > > > > > Could you skip this patch? > > > > > > > > I'm fine with either merging or dropping this. > > > > > > > > > > > > > > Let we review other patches firstly? > > > > > > > > I will be on vacation soon, and won't have time to do this until next week. > > > > > > Have a happly vacation. > > > > > > > > > > > But I spot two possible "issues": > > > > > > > > 1) the DMA metadata were stored in the headroom of the page, this > > > > breaks frags coalescing, we need to benchmark it's impact > > > > > > Not every page, just the first page of the COMP pages. > > > > > > So I think there is no impact. > > > > Nope, see this: > > > > if (SKB_FRAG_PAGE_ORDER && > > !static_branch_unlikely(&net_high_order_alloc_disable_key)) { > > /* Avoid direct reclaim but allow kswapd to wake */ > > pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) | > > __GFP_COMP | __GFP_NOWARN | > > __GFP_NORETRY, > > SKB_FRAG_PAGE_ORDER); > > if (likely(pfrag->page)) { > > pfrag->size = PAGE_SIZE << SKB_FRAG_PAGE_ORDER; > > return true; > > } > > } > > > > The comp page might be disabled due to the SKB_FRAG_PAGE_ORDER and > > net_high_order_alloc_disable_key. > > > YES. > > But if comp page is disabled. Then we only get one page each time. The pages are > not contiguous, so we don't have frags coalescing. > > If you mean the two pages got from alloc_page may be contiguous. The coalescing > may then be broken. It's a possibility, but I think the impact will be small.Let's have a simple benchmark and see? Thanks> > Thanks. > > > > > > > > > > > > > > 2) pre mapped DMA addresses were not reused in the case of XDP_TX/XDP_REDIRECT > > > > > > Because that the tx is not the premapped mode. > > > > Yes, we can optimize this on top. > > > > Thanks > > > > > > > > Thanks. > > > > > > > > > > > I see Michael has merge this series so I'm fine to let it go first. > > > > > > > > Thanks > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > >