net/virtio: enable packet data prefetch on x86

  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>
  

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.
  

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
  

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.
  

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
  

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.
  

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

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.
  

+ 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