[dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[Marvin Liu]

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)
-- 
2.17.1

Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[David Marchand]

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.

-- 
David Marchand

Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[Bruce Richardson]

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

Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[David Marchand]

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.


-- 
David marchand

Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[Maxime Coquelin]

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

Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[David Marchand]

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.

-- 
David Marchand

Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[Liu, Yong]

> -----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

Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[David Marchand]

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

Re: [dpdk-dev] [PATCH] net/virtio: enable packet data prefetch on x86

[Liu, Yong]

+ 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