Re: [dpdk-dev] [PATCH v7 2/4] virtio: Introduce config RTE_VIRTIO_INC_VECTOR

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2016-02-22 09:44, Santosh Shukla:
> On Mon, Feb 22, 2016 at 7:33 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
> > On 2/19/2016 2:42 PM, Yuanhan Liu wrote:
> >> On Fri, Feb 19, 2016 at 10:16:42AM +0530, Santosh Shukla wrote:
> >>> On Tue, Feb 16, 2016 at 8:35 AM, Yuanhan Liu
> >>> <yuanhan.liu@linux.intel.com> wrote:
> >>>> On Mon, Feb 15, 2016 at 04:48:36PM +0530, Santosh Shukla wrote:
> >>>>> Hi Yuanhan,
> >>>>>
> >>>>> On Mon, Feb 15, 2016 at 4:27 PM, Yuanhan Liu
> >>>>> <yuanhan.liu@linux.intel.com> wrote:
> >>>>>> I had a quick glimpse of the comments from Thomas: he made a good point.
> >>>>>> I will have a deeper thought tomorrow, to see what I can do to fix it.
> >>>>>>
> >>>>> I agree to what Thomas pointed out about runtime mode switch (vectored
> >>>>> vs non-vectored). I have a proposal in my mind and Like to know you
> >>>>> opinion:
> >>>>>
> >>>>> - need for apis like is_arch_support_vec().
> >>>>>
> >>>>> if (is_arch_support_vec())
> >>>>>          simpple_xxxx = 1 /* Switch code path to vector mode */
> >>>>> else
> >>>>>          simple_xxxx = 0  /* Switch code path to non-vector mode */
> >>>>>
> >>>>> That api should reside to arch file. i.e.. arch like i686/arm{for
> >>>>> implementation not exist so say no supported} will return 0 and for
> >>>>> x86_64 = 1
> >>>> I was thinking that Thomas meant to something like below (like what
> >>>> we did at rte_memcpy.h):
> >>>>
> >>>>     #ifdef RTE_MACHINE_CPUFLAG_SSE (or whatever)
> >>>>
> >>>>         /* with vec here */
> >>>>
> >>>>     #else
> >>>>
> >>>>         /* without vec here */
> >>>>
> >>>>     #endif
> >>>>
> >>>> I mean, you have to bypass the build first; otherwise, you can't
> >>>> go that further to runtime, right?
> >>>>
> >>> I meant: move virtio_recv_pkt_vec() implementation in
> >>> lib/libeal_rte/xx/include/arch/xx/virtio_vec.h. virtio driver to check
> >>> for CPUFLAG supported or not and then use _recv_pkt() call back
> >>> function from arch files. This approach will avoid #ifdef ARCH
> >>> clutter.
> >> Moving virtio stuff to eal looks wrong to me.
> >
> > This issue doesn't apply to virtio driver only but to all other PMDs,
> > unless they are assumed to run on only one arch. As we are close to
> > release, for the time being, i prefer to use RTE_MACHINE_CPUFLAG_.

Yes the obvious fix is to use some CPU flags.
In ACL the flags are checked on runtime to allow using some optimizations
after a "default" build. It can be considered later for virtio.

> > Later
> > we look for other elegant solutions, like moving different arch specific
> > optimizations into the arch directory under driver/virtio/ directory?
> > Other thoughts?
> 
> Creating arch specifics files in driver/virtio/: approach look okay to
> me. It look alike to my proposal except eal. I choose eal so that one
> api and its implementation stays in arch files, no ifdef clutter. I
> guess - Same doable in virtio directory too, create arch files and
> keep arch specific implementation their.
> 
> so, +1 to approach.

If there are some basic functions which can be re-used in other libs, there
must be in EAL. For virtio-specific functions, you can have some arch-specific
files in virtio.