DPDK patches and discussions
 help / color / mirror / Atom feed
From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: "Qiu, Michael" <michael.qiu@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v1] virtio: Use cpuflag for vector api
Date: Wed, 2 Mar 2016 10:49:18 +0800	[thread overview]
Message-ID: <20160302024918.GQ14300@yliu-dev.sh.intel.com> (raw)
In-Reply-To: <533710CFB86FA344BFBF2D6802E6028622F5AC3E@SHSMSX101.ccr.corp.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

  reply	other threads:[~2016-03-02  2:48 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-02-26  8:51 Santosh Shukla
2016-02-29  4:27 ` Yuanhan Liu
2016-02-29  5:33   ` Xie, Huawei
2016-02-29 12:31   ` Santosh Shukla
2016-03-01  5:55     ` Yuanhan Liu
2016-03-01  6:10       ` Santosh Shukla
2016-03-01  6:22         ` Yuanhan Liu
2016-02-29 12:58 ` [dpdk-dev] [PATCH v2] " Santosh Shukla
2016-03-01  5:59   ` Yuanhan Liu
2016-03-01  6:08     ` Santosh Shukla
2016-03-01  6:32       ` Yuanhan Liu
2016-03-01 10:07         ` Santosh Shukla
2016-03-01 10:02   ` [dpdk-dev] [PATCH v1 0/3] virtio vector and misc Santosh Shukla
2016-03-01 10:02     ` [dpdk-dev] [PATCH v1 1/3] virtio: use vector rx/tx for ssse cpuflag only Santosh Shukla
2016-03-01 10:02     ` [dpdk-dev] [PATCH v1 2/3] config: enable virtio for armv7/v8 Santosh Shukla
2016-03-01 10:02     ` [dpdk-dev] [PATCH v1 3/3] guide/release: add virtio for arm feature info Santosh Shukla
2016-03-02  8:32     ` [dpdk-dev] [PATCH v1 0/3] virtio vector and misc Yuanhan Liu
2016-03-02  8:41       ` Santosh Shukla
2016-03-03 13:26         ` Thomas Monjalon
2016-03-03 13:50           ` Santosh Shukla
2016-03-01  9:11 ` [dpdk-dev] [PATCH v1] virtio: Use cpuflag for vector api Qiu, Michael
2016-03-01  9:45   ` Santosh Shukla
2016-03-02  2:10     ` Qiu, Michael
2016-03-02  2:49       ` Yuanhan Liu [this message]
2016-03-04  6:36         ` Qiu, Michael

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160302024918.GQ14300@yliu-dev.sh.intel.com \
    --to=yuanhan.liu@linux.intel.com \
    --cc=dev@dpdk.org \
    --cc=michael.qiu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).