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

["Xie, Huawei" <huawei.xie@intel.com>]

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:
>>>>> 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.
> Moving virtio stuff to eal looks wrong to me.
>
> Huawei, please comment on this.
>
> 	--yliu
>

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_. Later
we look for other elegant solutions, like moving different arch specific
optimizations into the arch directory under driver/virtio/ directory?
Other thoughts?