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

[Santosh Shukla <sshukla@mvista.com>]

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:
>> > 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?
>

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.

This patch is blocking virtio-for-arm entry which is floating close to
month or so, if no taker for this topic then pl. let me know, I'll
propose a patch. Thanks!

>
> 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