diff mbox series

net/virtio: enable packet data prefetch on x86

Message ID 20201111154007.13426-1-yong.liu@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Maxime Coquelin
Headers show
Series net/virtio: enable packet data prefetch on x86 | expand

Checks

Context Check Description
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Liu, Yong Nov. 11, 2020, 3:40 p.m. UTC
Data prefetch instruction can preload data into cpu’s hierarchical
cache before data access. Virtio datapath utilized this feature for
data access acceleration. As config RTE_PMD_PACKET_PREFETCH was
discarded, now packet data prefetch is enabled based on architecture.

Signed-off-by: Marvin Liu <yong.liu@intel.com>

Comments

David Marchand Nov. 11, 2020, 3:45 p.m. UTC | #1
On Wed, Nov 11, 2020 at 4:40 PM Marvin Liu <yong.liu@intel.com> wrote:
>
> Data prefetch instruction can preload data into cpu’s hierarchical
> cache before data access. Virtio datapath utilized this feature for
> data access acceleration. As config RTE_PMD_PACKET_PREFETCH was
> discarded, now packet data prefetch is enabled based on architecture.

IIUC, this config item was set for all architectures under make compilation.

$ git grep RTE_PMD_PACKET_PREFETCH v20.08 config/
v20.08:config/common_base:CONFIG_RTE_PMD_PACKET_PREFETCH=y

Now that we switched to meson, it got lost and this patch only
restores it for x86.
Can other architectures check this?


Thanks.
Bruce Richardson Nov. 11, 2020, 3:53 p.m. UTC | #2
On Wed, Nov 11, 2020 at 04:45:25PM +0100, David Marchand wrote:
> On Wed, Nov 11, 2020 at 4:40 PM Marvin Liu <yong.liu@intel.com> wrote:
> >
> > Data prefetch instruction can preload data into cpu’s hierarchical
> > cache before data access. Virtio datapath utilized this feature for
> > data access acceleration. As config RTE_PMD_PACKET_PREFETCH was
> > discarded, now packet data prefetch is enabled based on architecture.
> 
> IIUC, this config item was set for all architectures under make compilation.
> 
> $ git grep RTE_PMD_PACKET_PREFETCH v20.08 config/
> v20.08:config/common_base:CONFIG_RTE_PMD_PACKET_PREFETCH=y
> 
> Now that we switched to meson, it got lost and this patch only
> restores it for x86.
> Can other architectures check this?
> 
If it was globally enabled before, it probably should just be added to
config/rte_config.h file.

/Bruce
David Marchand Nov. 11, 2020, 3:56 p.m. UTC | #3
On Wed, Nov 11, 2020 at 4:54 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Nov 11, 2020 at 04:45:25PM +0100, David Marchand wrote:
> > On Wed, Nov 11, 2020 at 4:40 PM Marvin Liu <yong.liu@intel.com> wrote:
> > >
> > > Data prefetch instruction can preload data into cpu’s hierarchical
> > > cache before data access. Virtio datapath utilized this feature for
> > > data access acceleration. As config RTE_PMD_PACKET_PREFETCH was
> > > discarded, now packet data prefetch is enabled based on architecture.
> >
> > IIUC, this config item was set for all architectures under make compilation.
> >
> > $ git grep RTE_PMD_PACKET_PREFETCH v20.08 config/
> > v20.08:config/common_base:CONFIG_RTE_PMD_PACKET_PREFETCH=y
> >
> > Now that we switched to meson, it got lost and this patch only
> > restores it for x86.
> > Can other architectures check this?
> >
> If it was globally enabled before, it probably should just be added to
> config/rte_config.h file.
>

Restoring globally is the probably the best fix, yes.
I am still surprised nobody but x86 testers caught a perf regression.
Maxime Coquelin Nov. 12, 2020, 8:48 a.m. UTC | #4
Hi Marvin,

On 11/11/20 4:40 PM, Marvin Liu wrote:
> Data prefetch instruction can preload data into cpu’s hierarchical
> cache before data access. Virtio datapath utilized this feature for
> data access acceleration. As config RTE_PMD_PACKET_PREFETCH was
> discarded, now packet data prefetch is enabled based on architecture.
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> 
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index 42c4c9882..0196290a5 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -106,7 +106,7 @@ virtqueue_store_flags_packed(struct vring_packed_desc *dp,
>  		dp->flags = flags;
>  	}
>  }
> -#ifdef RTE_PMD_PACKET_PREFETCH
> +#if defined(RTE_ARCH_X86)
>  #define rte_packet_prefetch(p)  rte_prefetch1(p)
>  #else
>  #define rte_packet_prefetch(p)  do {} while(0)
> 

Thanks for catching this issue.
I agree it should be re-enabled by default, and not only on X86, not
only on Virtio PMD.

AFAICS, prefetch was enabled for all platforms before the switch to
Meson, so I see it as an involuntary change that needs to be reverted.

Then, I think having a possibility to disable it would be nice, so maybe
we should add an option in our build system to do that.

Thanks,
Maxime
David Marchand Nov. 12, 2020, 8:57 a.m. UTC | #5
On Thu, Nov 12, 2020 at 9:48 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 11/11/20 4:40 PM, Marvin Liu wrote:
> > Data prefetch instruction can preload data into cpu’s hierarchical
> > cache before data access. Virtio datapath utilized this feature for
> > data access acceleration. As config RTE_PMD_PACKET_PREFETCH was
> > discarded, now packet data prefetch is enabled based on architecture.
> >
> > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >
> > diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> > index 42c4c9882..0196290a5 100644
> > --- a/drivers/net/virtio/virtqueue.h
> > +++ b/drivers/net/virtio/virtqueue.h
> > @@ -106,7 +106,7 @@ virtqueue_store_flags_packed(struct vring_packed_desc *dp,
> >               dp->flags = flags;
> >       }
> >  }
> > -#ifdef RTE_PMD_PACKET_PREFETCH
> > +#if defined(RTE_ARCH_X86)
> >  #define rte_packet_prefetch(p)  rte_prefetch1(p)
> >  #else
> >  #define rte_packet_prefetch(p)  do {} while(0)
> >
>
> Thanks for catching this issue.
> I agree it should be re-enabled by default, and not only on X86, not
> only on Virtio PMD.
>
> AFAICS, prefetch was enabled for all platforms before the switch to
> Meson, so I see it as an involuntary change that needs to be reverted.

Yes and this will also solve https://patchwork.dpdk.org/patch/83468/.
Thanks.
Liu, Yong Nov. 13, 2020, 1:20 a.m. UTC | #6
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, November 12, 2020 4:58 PM
> To: Liu, Yong <yong.liu@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; dev <dev@dpdk.org>; Richardson,
> Bruce <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on
> x86
> 
> On Thu, Nov 12, 2020 at 9:48 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
> > On 11/11/20 4:40 PM, Marvin Liu wrote:
> > > Data prefetch instruction can preload data into cpu’s hierarchical
> > > cache before data access. Virtio datapath utilized this feature for
> > > data access acceleration. As config RTE_PMD_PACKET_PREFETCH was
> > > discarded, now packet data prefetch is enabled based on architecture.
> > >
> > > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> > >
> > > diff --git a/drivers/net/virtio/virtqueue.h
> b/drivers/net/virtio/virtqueue.h
> > > index 42c4c9882..0196290a5 100644
> > > --- a/drivers/net/virtio/virtqueue.h
> > > +++ b/drivers/net/virtio/virtqueue.h
> > > @@ -106,7 +106,7 @@ virtqueue_store_flags_packed(struct
> vring_packed_desc *dp,
> > >               dp->flags = flags;
> > >       }
> > >  }
> > > -#ifdef RTE_PMD_PACKET_PREFETCH
> > > +#if defined(RTE_ARCH_X86)
> > >  #define rte_packet_prefetch(p)  rte_prefetch1(p)
> > >  #else
> > >  #define rte_packet_prefetch(p)  do {} while(0)
> > >
> >
> > Thanks for catching this issue.
> > I agree it should be re-enabled by default, and not only on X86, not
> > only on Virtio PMD.
> >
> > AFAICS, prefetch was enabled for all platforms before the switch to
> > Meson, so I see it as an involuntary change that needs to be reverted.
> 
> Yes and this will also solve https://patchwork.dpdk.org/patch/83468/.
> Thanks.
> 

Agreed, original patch was intended to recover prefetch configuration in meson build. 
Please check http://patchwork.dpdk.org/patch/78451/. 
And it leaded a discussion about how to utilize prefetch function optimally. 
Due to no conclusion for current position is best for other platforms except x86, now only enable prefetch in virtio + x86. 

> --
> David Marchand
David Marchand Nov. 13, 2020, 7:27 a.m. UTC | #7
On Fri, Nov 13, 2020 at 2:20 AM Liu, Yong <yong.liu@intel.com> wrote:
> > Yes and this will also solve https://patchwork.dpdk.org/patch/83468/.
> > Thanks.
> >
>
> Agreed, original patch was intended to recover prefetch configuration in meson build.
> Please check http://patchwork.dpdk.org/patch/78451/.
> And it leaded a discussion about how to utilize prefetch function optimally.
> Due to no conclusion for current position is best for other platforms except x86, now only enable prefetch in virtio + x86.

I disagree.
No conclusion means the best is to restore the previous state, i.e.
enable this option for all platforms.

If later other architectures want to change this, this can revisit.
Liu, Yong Nov. 13, 2020, 12:21 p.m. UTC | #8
+ more people into this conversation.  IMHO, restore to previous state is the best choice by now.

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, November 13, 2020 3:27 PM
> To: Liu, Yong <yong.liu@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; dev <dev@dpdk.org>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on
> x86
> 
> On Fri, Nov 13, 2020 at 2:20 AM Liu, Yong <yong.liu@intel.com> wrote:
> > > Yes and this will also solve https://patchwork.dpdk.org/patch/83468/.
> > > Thanks.
> > >
> >
> > Agreed, original patch was intended to recover prefetch configuration in
> meson build.
> > Please check http://patchwork.dpdk.org/patch/78451/.
> > And it leaded a discussion about how to utilize prefetch function
> optimally.
> > Due to no conclusion for current position is best for other platforms
> except x86, now only enable prefetch in virtio + x86.
> 
> I disagree.
> No conclusion means the best is to restore the previous state, i.e.
> enable this option for all platforms.
> 
> If later other architectures want to change this, this can revisit.
> 
> 
> --
> David Marchand
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 42c4c9882..0196290a5 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -106,7 +106,7 @@  virtqueue_store_flags_packed(struct vring_packed_desc *dp,
 		dp->flags = flags;
 	}
 }
-#ifdef RTE_PMD_PACKET_PREFETCH
+#if defined(RTE_ARCH_X86)
 #define rte_packet_prefetch(p)  rte_prefetch1(p)
 #else
 #define rte_packet_prefetch(p)  do {} while(0)