From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rich Lane Subject: Re: [PATCH v5 1/3] vhost: Add callback and private data for vhost PMD Date: Mon, 28 Dec 2015 13:59:59 -0800 Message-ID: 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> <567B61D6.1090806@igel.co.jp> <567BA5B9.5090006@igel.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: dev@dpdk.org, ann.zhuangyanying@huawei.com To: Tetsuya Mukawa Return-path: Received: from mail-vk0-f45.google.com (mail-vk0-f45.google.com [209.85.213.45]) by dpdk.org (Postfix) with ESMTP id 23D708F9E for ; Mon, 28 Dec 2015 23:00:00 +0100 (CET) Received: by mail-vk0-f45.google.com with SMTP id a188so185849424vkc.0 for ; Mon, 28 Dec 2015 14:00:00 -0800 (PST) In-Reply-To: <567BA5B9.5090006@igel.co.jp> 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 Wed, Dec 23, 2015 at 11:58 PM, Tetsuya Mukawa wrote: > > Hi Rich and Yuanhan, > > I guess we have 2 implementations here. > > 1. rte_eth_vhost_get_queue_event() returns each event. > 2. rte_eth_vhost_get_queue_status() returns current status of the queues. > > I guess option "2" is more generic manner to handle interrupts from > device driver. > In the case of option "1", if DPDK application doesn't call > rte_eth_vhost_get_queue_event(), the vhost PMD needs to keep all events. > This may exhaust memory. > Option 1 can be implemented in constant space by only tracking the latest state of each queue. I pushed a rough implementation to https://github.com/rlane/dpdk vhost-queue-callback. One more example is current link status interrupt handling. > Actually ethdev API just returns current status of the port. > What do you think? > Option 2 adds a small burden to the application but I'm fine with this as long as it's thread-safe (see my comments below). > > An issue with having the application dig into struct virtio_net is that > it > > can only be safely accessed from > > a callback on the vhost thread. > > Here is one of example how to invoke a callback handler registered by > DPDK application from the PMD. > > _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC); > > Above function is called by interrupt handling thread of the PMDs. > > Please check implementation of above function. > A callback handler that DPDK application registers is called in > "interrupt handling context". > (I mean the interrupt handling thread of the PMD calls the callback > handler of DPDK application also.) > Anyway, I guess the callback handler of DPDK application can access to > "struct virtio_net" safely. > > A typical application running its control > > plane on lcore 0 would need to > > copy all the relevant info from struct virtio_net before sending it over. > > Could you please describe it more? > Sorry, probably I don't understand correctly which restriction make you > copy data. > (As described above, the callback handler registered by DPDK application > can safely access "to struct virtio_net". Does this solve the copy issue?) > The ethdev event callback can safely access struct virtio_net, yes. The problem is that a real application will likely want to handle queue state changes as part of its main event loop running on a separate thread. Because no other thread can safely access struct virtio_net. the callback would need to copy the queue states out of struct virtio_net into another datastructure before sending it to the main thread. Instead of having the callback muck around with struct virtio_net, I would prefer an API that I could call from any thread to get the current queue states. This also removes struct virtio_net from the PMD's API which I think is a win. > > As you mentioned, queues for a single vhost port could be located on > > different NUMA nodes. I think this > > is an uncommon scenario but if needed you could add an API to retrieve > the > > NUMA node for a given port > > and queue. > > > > I agree this is very specific for vhost, because in the case of generic > PCI device, all queues of a port are on same NUMA node. > Anyway, because it's very specific for vhost, I am not sure we should > add ethdev API to handle this. > > If we handle it by vhost PMD API, we probably have 2 options also here. > > 1. Extend "struct rte_eth_vhost_queue_event , and use > rte_eth_vhost_get_queue_event() like you described. > struct rte_eth_vhost_queue_event > { > uint16_t queue_id; > bool rx; > bool enable; > + int socket_id; > }; > > 2. rte_eth_vhost_get_queue_status() returns current socket_ids of all > queues. > Up to you, but I think we can skip this for the time being because it would be unusual for a guest to place virtqueues for one PCI device on different NUMA nodes.