Re: [dpdk-dev] [PATCH v2 1/3] eal/arm64: relax the io barrier for aarch64

[Gavin Hu <Gavin.Hu@arm.com>]

Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Friday, January 3, 2020 3:35 PM
> To: Gavin Hu <Gavin.Hu@arm.com>
> Cc: dpdk-dev <dev@dpdk.org>; nd <nd@arm.com>; David Marchand
> <david.marchand@redhat.com>; thomas@monjalon.net;
> rasland@mellanox.com; maxime.coquelin@redhat.com; tiwei.bie@intel.com;
> hemant.agrawal@nxp.com; jerinj@marvell.com; Pavan Nikhilesh
> <pbhagavatula@marvell.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> <Joyce.Kong@arm.com>; Steve Capper <Steve.Capper@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/3] eal/arm64: relax the io barrier for
> aarch64
> 
> On Fri, Jan 3, 2020 at 12:00 PM Gavin Hu <Gavin.Hu@arm.com> wrote:
> >
> > Hi Jerin,
> 
> Hi Gavin,
> 
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Thursday, January 2, 2020 5:52 PM
> > > To: Gavin Hu <Gavin.Hu@arm.com>
> > > Cc: dpdk-dev <dev@dpdk.org>; nd <nd@arm.com>; David Marchand
> > > <david.marchand@redhat.com>; thomas@monjalon.net;
> > > rasland@mellanox.com; maxime.coquelin@redhat.com;
> tiwei.bie@intel.com;
> > > hemant.agrawal@nxp.com; jerinj@marvell.com; Pavan Nikhilesh
> > > <pbhagavatula@marvell.com>; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> > > <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce Kong
> > > <Joyce.Kong@arm.com>; Steve Capper <Steve.Capper@arm.com>
> > > Subject: Re: [dpdk-dev] [PATCH v2 1/3] eal/arm64: relax the io barrier for
> > > aarch64
> > >
> > > On Mon, Dec 23, 2019 at 3:46 PM Gavin Hu <Gavin.Hu@arm.com> wrote:
> > > >
> > > > Hi Jerin,
> > > >
> > > > > -----Original Message-----
> > > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > > Sent: Monday, December 23, 2019 5:20 PM
> > > > > To: Gavin Hu <Gavin.Hu@arm.com>
> > > > > Cc: dpdk-dev <dev@dpdk.org>; nd <nd@arm.com>; David Marchand
> > > > > <david.marchand@redhat.com>; thomas@monjalon.net;
> > > > > rasland@mellanox.com; maxime.coquelin@redhat.com;
> > > > > tiwei.bie@intel.com; hemant.agrawal@nxp.com; jerinj@marvell.com;
> > > > > Pavan Nikhilesh <pbhagavatula@marvell.com>; Honnappa Nagarahalli
> > > > > <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> > > > > <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>; Joyce
> Kong
> > > > > <Joyce.Kong@arm.com>; Steve Capper <Steve.Capper@arm.com>
> > > > > Subject: Re: [dpdk-dev] [PATCH v2 1/3] eal/arm64: relax the io barrier
> for
> > > > > aarch64
> > > > >
> > > > > On Mon, Dec 23, 2019 at 2:44 PM Gavin Hu <Gavin.Hu@arm.com>
> wrote:
> > > > > >
> > > > > > Hi Jerin,
> > > > >
> > > > > Hi Gavin,
> > > > >
> > > > > >
> > > > > > I think we are on the same page with regard to the problem, and the
> > > > > situations, thanks for illuminating the historical background of the two
> > > > > barriers.
> > > > > > About the solution, I added inline comments.
> > > > > > > It will be optimization only when if we are changing in the fast path.
> > > > > > > In the slow path, it does not matter.
> > > > > > > I think, the First step should be to use rte_cio_* wherever it is
> > > > > > > coherent memory used in _fast path_. I think, Almost every driver
> > > > > > > fixed that.
> > > > > > >
> > > > > > > I am not against this patch(changing the slow path to use rte_cio*
> > > > > > > from rte_io* and virtio changes associated with that).
> > > > > > > If you are taking that patch, pay attention to all the drivers in the
> > > > > > > tree which is using rte_io* for mixed access in slowpath.
> > > > > > I see 30+ drivers has calling rte_io* directly or indirectly through
> > > > > rte_write/read*.
> > > > > > It is hard for me to figure out all the mixed accesses in these drivers,
> and
> > > > > as you said, it makes no sense to change the _slow path_.
> > > > > >
> > > > > > How about we keep the old rte_io as is, and introduce 'fast path'
> version
> > > > > of rte_io for new code use?
> > > > > > Then in future, we may merge the two?
> > > > > > Another reason about this proposal is maybe there is rte_io calling in
> > > the
> > > > > fast path, but they are not mixed accesses and rte_cio is not suitable.
> > > > >
> > > > > Could you share more details about the case where fastpath + rte_io
> > > > > needed + rte_cio is not suitable?
> > > >
> > > > Here is an example for i40e, in the fast path, but only a pure io memory
> > > access.
> > > >
> > >
> https://code.dpdk.org/dpdk/v19.11/source/drivers/net/i40e/i40e_rxtx.c#L12
> > > 08
> > >
> > > Yes. That's a performance issue.
> > >
> > > It could be changed to following for the fix that works on x86, arm64
> > > with existing infra.
> > >
> > > From:
> > > I40E_PCI_REG_WRITE()
> > >
> > > to:
> > >
> > > rte_cio_wmb()
> > > I40E_PCI_REG_WRITE_RELAXED()
> > Yes, this is correct, I will submit a new patch for this.
> > This is an example out of all the cases that I must fix before relaxing the
> rte_io barriers.
> > My plan is as follows, any comments are welcome!
> > 1. replace rte_*mb and rte_io_*mb with rte_cio_*mb where applicable in
> the fastpath, this is an optimization, as the barriers are relaxed.
> > 2. replace all the rte_io_*mb with rte_cio_*mb where applicable in the
> slowpath and control path
> > 3. until *all* the occurrences in the step 1 and 2 are done, then this path can
> be re-activated.
> >
> > Please advise if the above approach works from your viewpoint.
> 
> I would prefer to have ONLY the step (1) and add a note for the same
> in  https://doc.dpdk.org/guides/prog_guide/perf_opt_guidelines.html
> as documentation reference.
Ok, good idea, I will submit a documentation patch for this. 
> 
> 
> > Maybe I will stop at step 1, step 2 and 3 are not necessary as they are not in
> the fastpath?
> 
> Yup.
> 
> >
> > >
> > > >
> > > > I wanted two variants of rte_io, because also x86 requires two as
> indicated
> > > here, one for no-WC and another for WC.
> > > > http://inbox.dpdk.org/dev/20191204151916.12607-1-
> > > xiaoyun.li@intel.com/T/#ea8bb1b4a378ab09baedbf95b4542bcb92f4a396f
> > > > >
> > > > > >
> > > > > > Any thoughts?
> > > > > >
> > > > > > >
> > > > > > > > But as the case in i40e, we must pay attention to where rte_cio
> was
> > > > > > > missing but rescued by old rte_io(but not by new rte_io).
> > > > > > > >
> > > > > > > >