DPDK patches and discussions
 help / color / mirror / Atom feed
From: Tiwei Bie <tiwei.bie@intel.com>
To: Olivier MATZ <olivier.matz@6wind.com>
Cc: dev@dpdk.org, Yuanhan Liu <yliu@fridaylinux.org>,
	Maxime Coquelin <maxime.coquelin@redhat.com>,
	Samuel Gauthier <samuel.gauthier@6wind.com>,
	stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix Rx and Tx handler selection for arm32
Date: Wed, 20 Dec 2017 09:31:10 +0800	[thread overview]
Message-ID: <20171220013110.urhbi7eagbrvwmkk@debian-xvivbkq> (raw)
In-Reply-To: <20171219105030.kazbbek5anz2js25@glumotte.dev.6wind.com>

Hi Olivier,

On Tue, Dec 19, 2017 at 11:50:31AM +0100, Olivier MATZ wrote:
> On Mon, Dec 18, 2017 at 10:50:47AM +0800, Tiwei Bie wrote:
> > On Thu, Dec 14, 2017 at 03:32:13PM +0100, Olivier Matz wrote:
> > > From: Samuel Gauthier <samuel.gauthier@6wind.com>
> > > 
> > > On arm32, we were always selecting the simple handler, but it is only
> > > available if neon is present.
> > > 
> > > This is due to a typo in the name of the config option.
> > > CONFIG_RTE_ARCH_ARM is for Makefiles. One should use RTE_ARCH_ARM.
> > > 
> > > Fixes: 2d7c37194ee4 ("net/virtio: add NEON based Rx handler")
> > > Cc: stable@dpdk.org
> > 
> > Hi Olivier,
> > 
> > My comment isn't really related to this patch, but related
> > to the commit it fixes and some related commits from you.
> > 
> > The commit (2d7c37194ee4) specified by the fixline doesn't
> > really cause the problem described in the commit log:
> > 
> > "On arm32, we were always selecting the simple handler, ..."
> > 
> > Actually, it will cause the simple handler won't be chosen
> > on arm32. Below is the relevant part (use_simple_rxtx won't
> > get a chance to be updated to 1 on arm32):
> > 
> > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> > index 0e369bd12..9ab441bc3 100644
> > --- a/drivers/net/virtio/virtio_rxtx.c
> > +++ b/drivers/net/virtio/virtio_rxtx.c
> > @@ -488,6 +488,9 @@ virtio_update_rxtx_handler(struct rte_eth_dev *dev,
> >  #if defined RTE_ARCH_X86
> >  	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE3))
> >  		use_simple_rxtx = 1;
> > +#elif defined RTE_ARCH_ARM64 || defined CONFIG_RTE_ARCH_ARM
> > +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> > +		use_simple_rxtx = 1;
> >  #endif
> >  	/* Use simple rx/tx func if single segment and no offloads */
> >  	if (use_simple_rxtx &&
> 
> You are right. The commit 2d7c37194ee4 introduces the typo in the config
> name, but does not trigger the same issue since the handler selection
> logic was reversed later.
> 
> I think we can keep this "Fixes: 2d7c37194ee4" but also add the other
> that you quote below.

Looks good to me. Or I think it's also OK to mainly talk
about the typo in the commit log without talking about the
exact effect to the Tx/Rx handler selection.

> 
> > It's the below commits (together with the above commit)
> > caused the simple handler will always be chosen on arm32:
> > 
> > 4819eae8d94b ("net/virtio: rationalize setting of Rx/Tx handlers")
> > 0964936308cd ("net/virtio: keep Rx handler whatever the Tx queue config")
> > 
> > For the above two commits, I think they have some other
> > problems. From my understanding, vector Rx function of
> > virtio PMD doesn't really follow the virtio spec. It
> > assumes the desc idx in the used ring will be written by
> > the backend in an expected order (i.e. the same order in
> > avail ring). So it even doesn't read the id field from
> > the used_elem to get the desc idx (but actually it should,
> > unless we change the virtio spec). It seems that simple
> > Tx function also has similar problem.
> 
> Ok, I was not aware of this.
> 
> > So IMO, we shouldn't choose the simple functions on all
> > platforms unless users enable them explicitly as we can't
> > guarantee they will work with all backends. So maybe the
> > problems can be fixed further? Any thoughts?
> 
> Yes, if the vector implementation does not follow the specifications, I
> think it should be fixed (prefered solution) or disabled by default. I'm
> wondering what would be the proper way to enable it, maybe it could be a
> device argument (rte_devargs)?

I'm not sure how to fix it because we don't want to see big
performance drop after the fix. If it can be fixed properly
in this release, it would be great. Otherwise, we may need
to choose the 2nd solution (i.e. disabled by default) first.
Adding devargs looks good to me.

Best regards,
Tiwei Bie

> 
> In that case, we could replace:
> 
>    hw->use_simple_rx = 1;
>    hw->use_simple_tx = 1;
> 
> by:
> 
>    hw->use_simple_rx = <value from devargs>;
>    hw->use_simple_tx = <value from devargs>;
> 
> 
> Olivier

      reply	other threads:[~2017-12-20  1:31 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-14 14:32 Olivier Matz
2017-12-14 14:45 ` Maxime Coquelin
2018-01-26 15:28   ` Yuanhan Liu
2017-12-18  2:50 ` Tiwei Bie
2017-12-19 10:50   ` Olivier MATZ
2017-12-20  1:31     ` Tiwei Bie [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171220013110.urhbi7eagbrvwmkk@debian-xvivbkq \
    --to=tiwei.bie@intel.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=olivier.matz@6wind.com \
    --cc=samuel.gauthier@6wind.com \
    --cc=stable@dpdk.org \
    --cc=yliu@fridaylinux.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).