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
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
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.
--
David marchand
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. -- David Marchand
> -----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.
--
David Marchand
+ 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