From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuanhan Liu Subject: Re: [PATCH v5 1/3] vhost: Add callback and private data for vhost PMD Date: Tue, 22 Dec 2015 13:47:10 +0800 Message-ID: <20151222054710.GK18863@yliu-dev.sh.intel.com> References: <1447392031-24970-3-git-send-email-mukawa@igel.co.jp> <1448355603-21275-1-git-send-email-mukawa@igel.co.jp> <1448355603-21275-2-git-send-email-mukawa@igel.co.jp> <20151217114223.GC29571@yliu-dev.sh.intel.com> <56737A5E.1030803@igel.co.jp> <20151218041536.GI29571@yliu-dev.sh.intel.com> <56738B5C.1030206@igel.co.jp> <20151222034158.GH18863@yliu-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org, ann.zhuangyanying@huawei.com To: Rich Lane Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 6B4AB5A24 for ; Tue, 22 Dec 2015 06:45:59 +0100 (CET) Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Dec 21, 2015 at 08:47:28PM -0800, Rich Lane wrote: > On Mon, Dec 21, 2015 at 7:41 PM, Yuanhan Liu > wrote: >=20 > On Fri, Dec 18, 2015 at 10:01:25AM -0800, Rich Lane wrote: > > I'm using the vhost callbacks and struct virtio_net with the vhos= t PMD in > a few > > ways: >=20 > Rich, thanks for the info! > =20 > > > > 1. new_device/destroy_device: Link state change (will be covered = by the > link > > status interrupt). > > 2. new_device: Add first queue to datapath. >=20 > I'm wondering why vring_state_changed() is not used, as it will als= o be > triggered at the beginning, when the default queue (the first queue= ) is > enabled. >=20 >=20 > Turns out I'd misread the code and it's already using the vring_state_c= hanged > callback for the > first queue. Not sure if this is intentional but vring_state_changed is= called > for the first queue > before new_device. Yeah, you were right, we can't count on this vring_state_changed(), for it's sent earlier than vring has been initialized on vhost side. Maybe we should invoke vring_state_changed() callback at new_device() as well. > =A0 >=20 > > 3. vring_state_changed: Add/remove queue to datapath. > > 4. destroy_device: Remove all queues (vring_state_changed is not = called > when > > qemu is killed). >=20 > I had a plan to invoke vring_state_changed() to disable all vrings > when destroy_device() is called. >=20 >=20 > That would be good. > =A0 >=20 > > 5. new_device and struct virtio_net: Determine NUMA node of the V= M. >=20 > You can get the 'struct virtio_net' dev from all above callbacks. >=20 > =A0 >=20 > > 1. Link status interrupt. >=20 > To vhost pmd, new_device()/destroy_device() equals to the link stat= us > interrupt, where new_device() is a link up, and destroy_device() is= link > down(). > =20 >=20 > > 2. New queue_state_changed callback. Unlike vring_state_changed t= his > should > > cover the first queue at new_device and removal of all queues at > > destroy_device. >=20 > As stated above, vring_state_changed() should be able to do that, e= xcept > the one on destroy_device(), which is not done yet. > =20 > > 3. Per-queue or per-device NUMA node info. >=20 > You can query the NUMA node info implicitly by get_mempolicy(); che= ck > numa_realloc() at lib/librte_vhost/virtio-net.c for reference. >=20 >=20 > Your suggestions are exactly how my application is already working. I w= as > commenting on the > proposed changes to the vhost PMD API. I would prefer to > use=A0RTE_ETH_EVENT_INTR_LSC > and=A0rte_eth_dev_socket_id for consistency with other NIC drivers, ins= tead of > these vhost-specific > hacks. That's a good suggestion. > The queue state change callback is the one new API that needs to be > added because > normal NICs don't have this behavior. Again I'd ask, will vring_state_changed() be enough, when above issues are resolved: vring_state_changed() will be invoked at new_device()/ destroy_device(), and of course, ethtool change? --yliu >=20 > You could add another=A0rte_eth_event_type for the queue state change c= allback, > and pass the > queue ID, RX/TX direction, and enable bit through cb_arg. The applicati= on would > never need > to touch struct virtio_net.