From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 6A33A4CBD for ; Thu, 28 Feb 2019 09:46:31 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CBBAC313433D; Thu, 28 Feb 2019 08:46:30 +0000 (UTC) Received: from [10.36.112.64] (ovpn-112-64.ams2.redhat.com [10.36.112.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8B41F68D82; Thu, 28 Feb 2019 08:46:29 +0000 (UTC) To: Ilya Maximets , dev@dpdk.org, changpeng.liu@intel.com, tiwei.bie@intel.com References: <20190227100235.14514-1-maxime.coquelin@redhat.com> <20190227100235.14514-3-maxime.coquelin@redhat.com> From: Maxime Coquelin Message-ID: Date: Thu, 28 Feb 2019 09:46:26 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Thu, 28 Feb 2019 08:46:30 +0000 (UTC) Subject: Re: [dpdk-dev] [RFC 2/2] vhost: support vhost-user request only handled by external backend X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Feb 2019 08:46:31 -0000 On 2/27/19 2:15 PM, Ilya Maximets wrote: > On 27.02.2019 13:02, Maxime Coquelin wrote: >> External backends may have specific requests to handle, and so >> we don't want the vhost-user lib to handle these requests as >> errors. >> >> This patch also catch the case where a request is neither handled >> by the external backend nor by the vhost library. >> >> Signed-off-by: Maxime Coquelin >> --- >> lib/librte_vhost/vhost_user.c | 28 +++++++++++++++------------- >> 1 file changed, 15 insertions(+), 13 deletions(-) >> >> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >> index 36c0c676d..bae5ef1cc 100644 >> --- a/lib/librte_vhost/vhost_user.c >> +++ b/lib/librte_vhost/vhost_user.c >> @@ -1924,27 +1924,29 @@ vhost_user_msg_handler(int vid, int fd) >> } >> >> ret = read_vhost_message(fd, &msg); >> - if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) { >> + if (ret <= 0) { >> if (ret < 0) >> RTE_LOG(ERR, VHOST_CONFIG, >> "vhost read message failed\n"); >> - else if (ret == 0) >> + else >> RTE_LOG(INFO, VHOST_CONFIG, >> "vhost peer closed\n"); >> - else >> - RTE_LOG(ERR, VHOST_CONFIG, >> - "vhost read incorrect message\n"); >> >> return -1; >> } >> >> ret = 0; >> - if (msg.request.master != VHOST_USER_IOTLB_MSG) >> - RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n", >> - vhost_message_str[msg.request.master]); >> - else >> - RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n", >> - vhost_message_str[msg.request.master]); >> + request = msg.request.master; >> + if (request < VHOST_USER_MAX && vhost_message_str[req]) { >> + if (request != VHOST_USER_IOTLB_MSG) >> + RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n", >> + vhost_message_str[request]); >> + else if ( >> + RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n", >> + vhost_message_str[request]); > > There is no need for the 'if' without the body. Oops, indeed. I was pretty sure I did compile test it, but looking at the history I didn't. >> + } else { >> + RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request); > > External requests could be annoying. Maybe we'll need to move them under DBG ? > I'm not sure. Fair point. I'll change to DBG. >> + } >> >> ret = vhost_user_check_and_alloc_queue_pair(dev, &msg); >> if (ret < 0) { >> @@ -1960,7 +1962,7 @@ vhost_user_msg_handler(int vid, int fd) >> * inactive, so it is safe. Otherwise taking the access_lock >> * would cause a dead lock. >> */ >> - switch (msg.request.master) { >> + switch (request) { >> case VHOST_USER_SET_FEATURES: >> case VHOST_USER_SET_PROTOCOL_FEATURES: >> case VHOST_USER_SET_OWNER: >> @@ -1985,6 +1987,7 @@ vhost_user_msg_handler(int vid, int fd) >> >> } >> >> + ret = RTE_VHOST_MSG_RESULT_ERR; > > This will break the 'vhost_crypto', because it has no 'pre_msg_handler' > and master will skip to 'post_msg_handler', but it will not be called > because current status is ERR. Thanks for catching it. > Maybe it's easier to introduce RTE_VHOST_MSG_RESULT_NOT_HANDLED and convert > it to ERR before the reply ? > This will require changing the pre_msg_handlers to return > RTE_VHOST_MSG_RESULT_NOT_HANDLED if message wasn't recognized. > And we'll possibly be able to drop the 'skip_master' in this case. Ok, that means breaking the API, but it is still experimental so not a blocker. I like the idea because it would also make it possible to add some debug prints. I'll post new iteration this morning. Thanks, Maxime >> if (dev->extern_ops.pre_msg_handle) { >> ret = (*dev->extern_ops.pre_msg_handle)(dev->vid, >> (void *)&msg, &skip_master); >> @@ -1997,7 +2000,6 @@ vhost_user_msg_handler(int vid, int fd) >> goto skip_to_post_handle; >> } >> >> - request = msg.request.master; >> if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) { >> if (!vhost_message_handlers[request]) >> goto skip_to_post_handle; >>