DPDK patches and discussions
 help / color / Atom feed
From: Li Feng <fengli@smartx.com>
To: Tiwei Bie <tiwei.bie@intel.com>
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>,
	Zhihong Wang <zhihong.wang@intel.com>,
	dev@dpdk.org,  Kyle Zhang <kyle@smartx.com>
Subject: Re: [dpdk-dev] [PATCH v3] vhost: add config change slave msg support
Date: Wed, 18 Dec 2019 14:34:59 +0800
Message-ID: <CAHckoCx=agrUHE7pCWBD374ddVVQMZKrQ0pqAqhM9JMiaBv94A@mail.gmail.com> (raw)
In-Reply-To: <20191216050559.GA129906@___>

Thanks.

Tiwei Bie <tiwei.bie@intel.com> 于2019年12月16日周一 下午1:05写道:
>
> On Thu, Dec 05, 2019 at 01:38:33PM +0800, Li Feng wrote:
> > This msg is used to notify qemu that should get the config of backend.
> >
> > For example, vhost-user-blk uses this msg to notify guest os the
> > compacity of backend has changed.
>
> capacity?
>
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v3:
> > * Move the declare to rte_vhost.h
> > * Add the symbol in rte_vhost_version.map
> >
> > v2:
> > * Fix a little log typo.
> >
> >  lib/librte_vhost/rte_vhost.h           | 12 ++++++++++++
> >  lib/librte_vhost/rte_vhost_version.map |  1 +
> >  lib/librte_vhost/vhost_user.c          | 31 +++++++++++++++++++++++++++++++
> >  lib/librte_vhost/vhost_user.h          |  1 +
> >  4 files changed, 45 insertions(+)
> >
> > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> > index 7b5dc87c2..fc28da264 100644
> > --- a/lib/librte_vhost/rte_vhost.h
> > +++ b/lib/librte_vhost/rte_vhost.h
> > @@ -977,6 +977,18 @@ __rte_experimental
> >  int
> >  rte_vhost_get_vdpa_device_id(int vid);
> >
> > +/**
> > + * Notify the guest that should get config from backend.
> > + *
> > + * @param vid
> > + *  vhost device ID
> > + * @return
> > + *  0 on success, < 0 on failure
> > + */
> > +__rte_experimental
> > +int
> > +rte_vhost_user_slave_config_change(int vid);
>
> Normally the prefix in vhost API is rte_vhost_
> not rte_vhost_user_
>
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
> > index c512377fe..acf013d6d 100644
> > --- a/lib/librte_vhost/rte_vhost_version.map
> > +++ b/lib/librte_vhost/rte_vhost_version.map
> > @@ -65,4 +65,5 @@ EXPERIMENTAL {
> >       rte_vhost_clr_inflight_desc_packed;
> >       rte_vhost_get_vhost_ring_inflight;
> >       rte_vhost_get_vring_base_from_inflight;
> > +     rte_vhost_user_slave_config_change;
> >  };
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 0cfb8b792..10f2e47d5 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -2840,6 +2840,37 @@ vhost_user_iotlb_miss(struct virtio_net *dev, uint64_t iova, uint8_t perm)
> >       return 0;
> >  }
> >
> > +static int
> > +vhost_user_slave_config_change(struct virtio_net *dev)
> > +{
> > +     int ret;
> > +     struct VhostUserMsg msg = {
> > +             .request.slave = VHOST_USER_SLAVE_CONFIG_CHANGE_MSG,
> > +             .flags = VHOST_USER_VERSION,
>
> Will it be better to also set VHOST_USER_NEED_REPLY?
This feature could not be supported because the vhost thread will
block at the read slave fd,
but qemu have sent a get_config message, and wait for the vhost thread
response, the stack like this:

#2  0x0000555555b435d2 in tcp_chr_recv (chr=chr@entry=0x555556b12c80,
buf=buf@entry=0x7fffffffd7c0 "\030", len=len@entry=12) at
chardev/char-socket.c:332
#3  0x0000555555b44611 in tcp_chr_sync_read (chr=0x555556b12c80,
buf=0x7fffffffd7c0 "\030", len=12) at chardev/char-socket.c:543
#4  0x0000555555b3f5e1 in qemu_chr_fe_read_all
(be=be@entry=0x555557e82310, buf=buf@entry=0x7fffffffd7c0 "\030",
len=len@entry=12) at chardev/char-fe.c:72
#5  0x0000555555884b7b in vhost_user_read_header (dev=0x555557e82310,
msg=0x7fffffffd7c0) at
/usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:236
#6  vhost_user_read (msg=msg@entry=0x7fffffffd7c0, dev=0x555557e82398)
at /usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:261
#7  0x00005555558850ff in vhost_user_get_config (dev=0x555557e82398,
config=0x7fffffffda80 "\340(6WUU", config_len=60)
    at /usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:1636
#8  0x0000555555842880 in vhost_user_blk_handle_config_change
(dev=0x555557e82398) at
/usr/src/debug/qemu-4.1.0/hw/block/vhost-user-blk.c:85
#9  0x0000555555886df1 in vhost_user_slave_handle_config_change
(dev=0x555557e82398) at
/usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:912
#10 slave_read (opaque=0x555557e82398) at
/usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:1050

>
> > +             .size = 0,
> > +     };
> > +
> > +     ret = send_vhost_message(dev->slave_req_fd, &msg);
> > +     if (ret < 0) {
> > +             RTE_LOG(ERR, VHOST_CONFIG,
> > +                             "Failed to send config change (%d)\n",
> > +                             ret);
>
> Looks better to indent the code by 3 tabs.
>
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +int
> > +rte_vhost_user_slave_config_change(int vid)
> > +{
> > +     struct virtio_net *dev;
> > +     dev = get_device(vid);
> > +     if (!dev)
> > +             return -ENODEV;
> > +     return vhost_user_slave_config_change(dev);
> > +}
>
> I'm wondering will it be better to provide a generic API
> to allow external backends to send any slave messages?
I have thought about this before, and I don't choose external backend because:
1. Sending slave message is more like an API, and external backend
acts like a hooker that receives messages from master;
If adding a new slave message type, we should also need to add type
definition in the dpdk librte_vhost header.
2. There is only three slave message type;


>
> > +
> >  static int vhost_user_slave_set_vring_host_notifier(struct virtio_net *dev,
> >                                                   int index, int fd,
> >                                                   uint64_t offset,
> > diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> > index 6563f7315..86c364a93 100644
> > --- a/lib/librte_vhost/vhost_user.h
> > +++ b/lib/librte_vhost/vhost_user.h
> > @@ -62,6 +62,7 @@ typedef enum VhostUserRequest {
> >  typedef enum VhostUserSlaveRequest {
> >       VHOST_USER_SLAVE_NONE = 0,
> >       VHOST_USER_SLAVE_IOTLB_MSG = 1,
> > +     VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
> >       VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
> >       VHOST_USER_SLAVE_MAX
> >  } VhostUserSlaveRequest;
> > --
> > 2.11.0
> >
> >
> > --
> > The SmartX email address is only for business purpose. Any sent message
> > that is not related to the business is not authorized or permitted by
> > SmartX.
> > 本邮箱为北京志凌海纳科技有限公司(SmartX)工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
> >
> >

-- 
The SmartX email address is only for business purpose. Any sent message 
that is not related to the business is not authorized or permitted by 
SmartX.
本邮箱为北京志凌海纳科技有限公司(SmartX)工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.



  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-02 14:57 [dpdk-dev] [PATCH] " Li Feng
2019-12-04  5:13 ` [dpdk-dev] [PATCH v2] " Li Feng
2019-12-04  9:30   ` Tiwei Bie
2019-12-04  9:43     ` Li Feng
2019-12-05  2:07       ` Tiwei Bie
2019-12-05  4:01         ` Li Feng
2019-12-05  4:57           ` Tiwei Bie
2019-12-05  5:38 ` [dpdk-dev] [PATCH v3] " Li Feng
2019-12-11  2:58   ` Li Feng
2019-12-16  5:06   ` Tiwei Bie
2019-12-18  6:34     ` Li Feng [this message]
2019-12-19  8:54 ` [dpdk-dev] [PATCH v4] " Li Feng
2019-12-20  8:04   ` Tiwei Bie
2019-12-20  8:22 ` [dpdk-dev] [PATCH v5] " Li Feng
2020-01-14 14:57   ` Maxime Coquelin
2020-01-15 11:18   ` Maxime Coquelin

Reply instructions:

You may reply publically 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='CAHckoCx=agrUHE7pCWBD374ddVVQMZKrQ0pqAqhM9JMiaBv94A@mail.gmail.com' \
    --to=fengli@smartx.com \
    --cc=dev@dpdk.org \
    --cc=kyle@smartx.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=tiwei.bie@intel.com \
    --cc=zhihong.wang@intel.com \
    /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

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox