Re: [PATCH v1] virtio: Use cpuflag for vector api

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

On Wed, Mar 02, 2016 at 02:10:14AM +0000, Qiu, Michael wrote:
> On 3/1/2016 5:46 PM, Santosh Shukla wrote:
> > On Tue, Mar 1, 2016 at 2:41 PM, Qiu, Michael <michael.qiu@intel.com> wrote:
> >> On 2/26/2016 4:53 PM, Santosh Shukla wrote:
> >>> Check cpuflag macro before using vectored api.
> >>> -virtio_recv_pkts_vec() uses _sse3__ simd instruction for now so added cpuflag.
> >>> - Also wrap other vectored freind api ie..
> >>> 1) virtqueue_enqueue_recv_refill_simple
> >>> 2) virtio_rxq_vec_setup
> >>>
> >>> todo:
> >>> 1) Move virtio_recv_pkts_vec() implementation to
> >>>    drivers/virtio/virtio_vec_<arch>.h file.
> >>> 2) Remove use_simple_rxtx flag, so that virtio/virtio_vec_<arch>.h
> >>>    files to provide vectored/non-vectored rx/tx apis.
> >>>
> >>> Signed-off-by: Santosh Shukla <sshukla@mvista.com>
> >>> ---
> >>> - v1: This is a rework of patch [1].
> >>> Note: This patch will let non-x86 arch to use virtio pmd.
> >>>
> >>> [1] http://dpdk.org/dev/patchwork/patch/10429/
> >>>
> >>>  drivers/net/virtio/virtio_rxtx.c        |   16 +++++++++++++++-
> >>>  drivers/net/virtio/virtio_rxtx.h        |    2 ++
> >>>  drivers/net/virtio/virtio_rxtx_simple.c |   11 ++++++++++-
> >>>  3 files changed, 27 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> >>> index 41a1366..ec0b8de 100644
> >>> --- a/drivers/net/virtio/virtio_rxtx.c
> >>> +++ b/drivers/net/virtio/virtio_rxtx.c
> >>> @@ -67,7 +67,9 @@
> >>>  #define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
> >>>       ETH_TXQ_FLAGS_NOOFFLOADS)
> >>>
> >>> +#ifdef RTE_MACHINE_CPUFLAG_SSSE3
> >>>  static int use_simple_rxtx;
> >>> +#endif
> >>>
> >>>
> >> I don't think so much #ifdef ... #endif in *.c file is a good choice.
> >> Would you consider let it only in header file like:
> >>
> >> in drivers/net/virtio/virtio_rxtx.h
> >>
> >> [...]
> >>
> >> #ifdef RTE_MACHINE_CPUFLAG_SSSE3
> >> int virtio_rxq_vec_setup(struct virtqueue *rxq);
> >>
> >> int virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
> >>         struct rte_mbuf *m);
> >> #else
> >> int virtio_rxq_vec_setup(__rte_unused struct virtqueue *rxq) {return -1;}
> >> int virtqueue_enqueue_recv_refill_simple(__rte_unused struct virtqueue *vq,
> >>                                          __rte_unused struct rte_mbuf *m) {
> >>         return -1;
> >> }
> >> #endif
> >>
> >> and remove most #ifdef ... #endif in *.c file.
> >>
> > I guess, above approach wont work for non-x86 arch, ad those func are
> > dummy, right? also code wont build for arm/non-86 arch because
> > tx/rx_pkt_burst callback will be using x86 specific virtio vec rx/tx
> > api.
> 
> You may right, but you really need to reduce the #ifdef in *.c files.

In general, yes. But for this case, no: those vec stuff are for
platforms that support it. For other platforms, we should not
invoke a dummy function like virtio_rxq_vec_setup() at all.

The right way to go is to add another wrapper beyond the vector
stuff, something like:

	virtio_rxq_setup()
	{

		if (has_vec_support)
			virtio_rxq_vec_setup();
		else
			virtio_rxq_generic_setup();
	}

Where virtio_rxq_vec_setup() could have a per-arch implementation,
say for X86, or ARM.

It touchs more code, but for now, I'd like to make it simple first.
With the virtio_rxtx_simple.c isolated from Makefile, there aren't
many #ifdef after all.

	--yliu