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

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

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:
> > On Mon, Feb 15, 2016 at 03:22:11PM +0530, Santosh Shukla wrote:
> >> Hi Yuanhan,
> >>
> >> I guess you are back from vacation.
> >>
> >> Can you pl. review this patch, Except this patch, rest of patches
> >> received ack-by:
> >
> > 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?


Huawei, since it's your patch introduced such issue, mind to fix
it?

	--yliu
> 
> Does this make sense?
> 
> Thanks
> >         --yliu
> >>
> >> Thanks
> >>
> >> On Mon, Feb 8, 2016 at 11:15 AM, Santosh Shukla <sshukla@mvista.com> wrote:
> >> > On Mon, Feb 8, 2016 at 2:55 AM, Thomas Monjalon
> >> > <thomas.monjalon@6wind.com> wrote:
> >> >> 2016-02-07 19:21, Santosh Shukla:
> >> >>> - virtio_recv_pkts_vec and other virtio vector friend apis are written for
> >> >>>   sse/avx instructions. For arm64 in particular, virtio vector implementation
> >> >>>   does not exist(todo).
> >> >>>
> >> >>> So virtio pmd driver wont build for targets like i686, arm64.  By making
> >> >>> RTE_VIRTIO_INC_VECTOR=n, Driver can build for non-sse/avx targets and will work
> >> >>> in non-vectored virtio mode.
> >> >>>
> >> >>> Disabling RTE_VIRTIO_INC_VECTOR config for :
> >> >>>
> >> >>> - i686 arch as i686 target config says:
> >> >>>   config/defconfig_i686-native-linuxapp-gcc says "Vectorized PMD is not
> >> >>>   supported on 32-bit".
> >> >>>
> >> >>> - armv7/v8 arch.
> >> >>
> >> >> Yes it can be useful to disable vector optimizations, but it should done
> >> >> at runtime, not a compilation option. I know it is already wrongly configured
> >> >> at compilation for other drivers, we should fix them.
> >> >>
> >> >
> >> > Can't we consider this separate topic. My intent is virtio works for arm.
> >> >
> >> >> Here, you want to avoid SSE/AVX code on ARM. So we should just add the
> >> >> appropriate ifdefs. Adding a compilation option does not prevent from enabling
> >> >> it on ARM or old x86 which do not support these instructions.
> >> >>
> >> >
> >> > By disabling VIRTIO_INC_VEC, compiler wont build
> >> > virtio_recv_pkts_vec(), so wont generate SSE/AVX code. Adding ifdef
> >> > for other arch example arm, is next step. Vector instruction for arm
> >> > are not fully supported, Its a todolist (Pl. refer my early v1/2
> >> > cover-letter), We'll add that after virtio functionally works for arm.
> >> >
> >> >> Please virtio maintainers, we need to fix this code. Thanks