DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Zhang, Roy Fan" <roy.fan.zhang@intel.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"Kulasek, TomaszX" <tomaszx.kulasek@intel.com>,
	"Wodkowski, PawelX" <pawelx.wodkowski@intel.com>
Cc: "jianjay.zhou@huawei.com" <jianjay.zhou@huawei.com>,
	"yliu@fridaylinux.org" <yliu@fridaylinux.org>,
	"Tan, Jianfeng" <jianfeng.tan@intel.com>,
	"Bie, Tiwei" <tiwei.bie@intel.com>
Subject: Re: [dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user private info structure
Date: Wed, 21 Mar 2018 16:11:31 +0000	[thread overview]
Message-ID: <9F7182E3F746AB4EA17801C148F3C60433102B6A@IRSMSX101.ger.corp.intel.com> (raw)
In-Reply-To: <87a9b225-dfa0-adbd-5a5e-483a1dcd0027@redhat.com>

Hi Maxime,

Thanks a lot for the fast reply. 

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, March 21, 2018 1:03 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org; Kulasek,
> TomaszX <tomaszx.kulasek@intel.com>; Wodkowski, PawelX
> <pawelx.wodkowski@intel.com>
> Cc: jianjay.zhou@huawei.com; yliu@fridaylinux.org; Tan, Jianfeng
> <jianfeng.tan@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user
> private info structure
> 
> Hi,
> 
> On 03/21/2018 10:10 AM, Zhang, Roy Fan wrote:
> > Hi Maxime,
> >
> > Thanks for the review.
> >
> >>
> >> I think this include isn't needed looking at the rest of the patch.
> >
> > I agree. I will remove this line here.
> >
> >>>    #define VHOST_USER_VERSION    0x1
> >>>
> >>> +typedef int (*msg_handler)(struct virtio_net *dev, struct
> >>> +VhostUserMsg
> >> *msg,
> >>> +		int fd);
> >>> +
> >>> +struct vhost_user_dev_priv {
> >>> +	msg_handler vhost_user_msg_handler;
> >>> +	char data[0];
> >>> +};
> >>>
> >>>    /* vhost_user.c */
> >>>    int vhost_user_msg_handler(int vid, int fd);
> >>>
> >>
> >> I think the wording is a bit misleading, I'm fine with having a
> >> private_data pointer, but it should only be used by the external backend.
> >>
> >> Maybe what you need here is a new API to be to register a callback
> >> for the external backend to handle specific requests.
> >
> > That's exactly what I need.
> > Shall I rework the code like this?
> 
> These new API are to be placed in rte_vhost.h, else it won't be exported.
> 
> > /* vhost.h */
> > struct virtio_net {
> > 	....
> > 	void *extern_data; /*<< private data for external backend */
> >
> > }
> 
> Looks good, you may need to add getter and setter APIs as struct virtio_net
> isn't part of the API.
> 
> > /* vhost_user.h */
> > typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg
> *msg,
> > 		int fd);
> 
> I wonder if the fd is really necessary, as if a reply is to be sent, it can be done
> by the vhost lib.

Same as the messages VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_GET_VRING_BASE etc, the external backend such as vhost crypto will require to send an reply independently. 
In case of vhost crypto, the message create_session will require the backend to send a session id back to the frontend. 

I tried to set VHOST_USER_NEED_REPLY bit in the vhost crypto message handler but it will cause problem, qemu returns " qemu-system-x86_64: Received bad msg size."

Another solution is to pass a variable back from the message handler to indicate sending the reply instead.
So the function prototype can be 
typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg *msg, uint32_t *require_reply);


> > struct vhost_user_dev_extern {
> > 	msg_handler post_vhost_user_msg_handler;
> > 	char data[0];
> Why is this filed needed?

Either this way, or using a pointer. The idea is to allocate contiguous memory when creating new vhost_user_dev_extern structure. The former way is slightly more performant to avoid another memory read, but I can live with the pointer instead :-)

> > };
> I would change this to:
> struct rte_vhost_user_dev_extern_ops {
> 	rte_vhost_msg_handler pre_vhost_user_msg_handler;
> 	rte_vhost_msg_handler post_vhost_user_msg_handler; };
> 
> > int
> > vhost_user_register_call_back(struct virtio_net *dev, msg_handler
> > post_msg_handler);
> 
> and something like:
> rte_vhost_user_register_extern_ops(struct virtio_net *dev, struct
> 				rte_vhost_user_dev_extern_ops *ops);

Great idea!

> >>
> >> Also, it might be interesting for the external backend to register
> >> callbacks for existing requests. For example
> >> .pre_vhost_user_msg_handler and .post_vhost_user_msg_handler.
> Doing
> >> so, the external backend could for example catch beforehand any
> >> change that could affect resources being used. Tomasz, Pawel, do you
> think that could help for the issue you reported?
> >>
> >
> >
> > I think it is definitely a good idea. However there will be a problem. As
> vhost_crypto does not require pre_vhost_user_msg_handler I think it may
> not appropriate to add pre_vhost_user_msg_handler in this patchset.
> 
> I see, but please add the pre_ callback directly in this series, as we know it
> will be useful (thanks Pawel).
> It will avoid breaking the API again when we'll need it.

Will do.

> 
> Thanks,
> Maxime
> > Thanks a million Maxime.
> >
> >> Thanks,
> >> Maxime

Thanks,
Fan

  reply	other threads:[~2018-03-21 16:11 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-27 16:29 [dpdk-dev] [PATCH v2 00/10] lib/librte_vhost: introduce new vhost user crypto backend support Fan Zhang
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user private info structure Fan Zhang
2018-03-13  9:08   ` Maxime Coquelin
2018-03-21  9:10     ` Zhang, Roy Fan
2018-03-21 11:41       ` Wodkowski, PawelX
2018-03-21 13:02       ` Maxime Coquelin
2018-03-21 16:11         ` Zhang, Roy Fan [this message]
2018-03-22  8:36           ` Wodkowski, PawelX
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 02/10] lib/librte_vhost: add virtio-crypto user message structure Fan Zhang
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 03/10] lib/librte_vhost: add session message handler Fan Zhang
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 04/10] lib/librte_vhost: add request handler Fan Zhang
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 05/10] lib/librte_vhost: add head file Fan Zhang
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 06/10] lib/librte_vhost: add public function implementation Fan Zhang
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 07/10] lib/librte_vhost: update version map Fan Zhang
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 08/10] lib/librte_vhost: update makefile Fan Zhang
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 09/10] examples/vhost_crypto: add vhost crypto sample application Fan Zhang
2018-02-27 16:29 ` [dpdk-dev] [PATCH v2 10/10] doc: update prog guide and sample app guid Fan Zhang

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=9F7182E3F746AB4EA17801C148F3C60433102B6A@IRSMSX101.ger.corp.intel.com \
    --to=roy.fan.zhang@intel.com \
    --cc=dev@dpdk.org \
    --cc=jianfeng.tan@intel.com \
    --cc=jianjay.zhou@huawei.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=pawelx.wodkowski@intel.com \
    --cc=tiwei.bie@intel.com \
    --cc=tomaszx.kulasek@intel.com \
    --cc=yliu@fridaylinux.org \
    /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).