From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id C914F5320 for ; Mon, 14 Mar 2016 09:20:47 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 14 Mar 2016 01:20:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,334,1455004800"; d="scan'208";a="909703970" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.66.49]) by orsmga001.jf.intel.com with ESMTP; 14 Mar 2016 01:20:45 -0700 Date: Mon, 14 Mar 2016 16:21:25 +0800 From: Yuanhan Liu To: Tetsuya Mukawa Cc: dev@dpdk.org, jianfeng.tan@intel.com, huawei.xie@intel.com Message-ID: <20160314082125.GM979@yliu-dev.sh.intel.com> References: <1457590462-3680-1-git-send-email-mukawa@igel.co.jp> <1457593565-16240-1-git-send-email-mukawa@igel.co.jp> <20160311071907.GI979@yliu-dev.sh.intel.com> <56E619C6.6060809@igel.co.jp> <20160314020858.GJ979@yliu-dev.sh.intel.com> <56E66E18.4000700@igel.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56E66E18.4000700@igel.co.jp> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2] vhost: Fix default value of kickfd and callfd X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Mar 2016 08:20:48 -0000 On Mon, Mar 14, 2016 at 04:54:00PM +0900, Tetsuya Mukawa wrote: > On 2016/03/14 11:08, Yuanhan Liu wrote: > > On Mon, Mar 14, 2016 at 10:54:14AM +0900, Tetsuya Mukawa wrote: > >> On 2016/03/11 16:19, Yuanhan Liu wrote: > >>> On Thu, Mar 10, 2016 at 04:06:05PM +0900, Tetsuya Mukawa wrote: > >>>> Currently, default values of kickfd and callfd are -1. > >>>> If the values are -1, current code guesses kickfd and callfd haven't > >>>> been initialized yet. And vhost library will guess the virtqueue isn't > >>>> ready for processing. > >>>> But callfd and kickfd will be set as -1 when "--enable-kvm" > >>>> isn't specified in QEMU command line. It means we cannot treat -1 as > >>>> uninitialized state. The patch changes default values to -2. And the > >>>> patch defines -2 as VIRTIO_UNINITIALIZED_EVENTFD. > >>> This looks more like a workaround to me. > >> > >> Hi Yuanhan, > >> > >> Sorry for late reply. > >> I have checked QEMU documentation, and found below. > >> > >> ---------- > >> * VHOST_USER_SET_VRING_CALL > >> > >> Id: 14 > >> Equivalent ioctl: VHOST_SET_VRING_CALL > >> Master payload: u64 > >> > >> Set the event file descriptor to signal when buffers are used. It > >> is passed in the ancillary data. > >> Bits (0-7) of the payload contain the vring index. Bit 8 is the > >> invalid FD flag. > >> ---------- > >> > >> VHOST_USER_SET_VRING_KICK has almost same description. > >> I will check this invalid flag, and if it works for our case, then will > >> use it. > >> How about it? > > Yeah, that indeed sounds much better. > > I've checked current dpdk code. > It seems we've already checked invalid flag like below. > > if (pmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK) > file.fd = -1; > else > file.fd = pmsg->fds[0]; > > So how about adding below macros or enum? > > #define VIRTIO_UNINITIALIZED_EVENTFD (-2) > #define VIRTIO_INVALID_EVENTFD (-1) > > I am still not sure whether using enum is better or not. Both are Okay to me; I have no preference on that. > But here is one of example patch. > What do you think? Looks okay to me > > diff --git a/lib/librte_vhost/rte_virtio_net.h > b/lib/librte_vhost/rte_virtio_net.h > index 7d1fde2..2a7566d 100644 > --- a/lib/librte_vhost/rte_virtio_net.h > +++ b/lib/librte_vhost/rte_virtio_net.h > @@ -89,6 +89,8 @@ struct vhost_virtqueue { > uint16_t vhost_hlen; /**< Vhost > header length (varies depending on RX merge buffers. */ > volatile uint16_t last_used_idx; /**< Last index > used on the available ring */ > volatile uint16_t last_used_idx_res; /**< Used for > multiple devices reserving buffers. */ > +#define VIRTIO_UNINITIALIZED_EVENTFD (-2) > +#define VIRTIO_INVALID_EVENTFD (-1) One nit: you may keep it in order. --yliu