From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 29F0FA0C52; Wed, 24 Nov 2021 15:01:46 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 17ECC41181; Wed, 24 Nov 2021 15:01:46 +0100 (CET) Received: from mail-qk1-f172.google.com (mail-qk1-f172.google.com [209.85.222.172]) by mails.dpdk.org (Postfix) with ESMTP id 28A5E41168 for ; Wed, 24 Nov 2021 15:01:45 +0100 (CET) Received: by mail-qk1-f172.google.com with SMTP id p4so2878118qkm.7 for ; Wed, 24 Nov 2021 06:01:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Id+uhqI9/3N2U0PShZW1vfG6k92XGstYybb/hOMbJas=; b=GoQhJIg90fT92mQEmhx9Q7ZR0t9r5QM5D4sUmxj2YDiO++5QnQNIorg5Rjq/2aoYnb SDM+P+wjupRGfNOcyQexRUNrnhcEMVewDzS6syDUwS5Ax3RbyLx5lvePlqctOg8w5Wi5 rDIle+2revl13AYlL5BER7//xCFAkJ52s4cvyNI3PTEZ2dmKmIGHgxSDa9QEXbMS2tMI J0iuXfMu4vLBL/SG4A51df62breQV71VEJ9ZqNSueGceGkYZuETimkki+vD6b9aqGa3g Os6PWNegki68qfYrGEPdVRnw76NcfjzxdaWI4vjm8r3e1XFxaAmef0KPnBnOVkWrSPO4 iFBQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Id+uhqI9/3N2U0PShZW1vfG6k92XGstYybb/hOMbJas=; b=qW6Amjk9CdfC0S2svaHEiGWicjQMPVi3eWAshOeZbfthrSCkuGDMZAfRtMbiCgGS6L W7sFxI3StWnKA6gCE97lkRPKCnpI0XMS84Vx4rPE7iw2RRlmka9CtAvQ2JsoFkWhW2lx 2S0tyIePNddINMZ1lT8v0+P8XDLTR9Qt39q1sCusCssmdT6Pgxo0RcjKOwHjQ4lz3Cxc EGWQC6sEF/JR+NBzT3KnAEpE1FKP0Yl55oH4bJVFxACLPA+ngUS6fqTtUPnueMtnhcvV 2jznk7jNfgWPfo1FzZNglcdF01e/07R69ijw6EAlUK2c4liEnackvP7Zpv1gfxxeA41U i6eQ== X-Gm-Message-State: AOAM531XDwHiIO8xSbco3Z++iu2B9BAAVwfzIqMHdz29XGXButUAmWED THT3VCnSHWrPXciLWk4x1hHHle0v2Ofa2ZyN0RM= X-Google-Smtp-Source: ABdhPJzvs+D5QVRzBanJojxaEzByW9rE2p2es9tdKeMxwDETvMLssCKzSFwPeEm4UhOsaGuOS0Y3aKsM+wui3chcy70= X-Received: by 2002:a05:620a:4245:: with SMTP id w5mr4609899qko.469.1637762504579; Wed, 24 Nov 2021 06:01:44 -0800 (PST) MIME-Version: 1.0 References: <119df28c7cc9d267@cs.arizona.edu> <57a1f49b-8797-48ee-9ec7-2ec60ad5a042@intel.com> In-Reply-To: <57a1f49b-8797-48ee-9ec7-2ec60ad5a042@intel.com> From: yoursunny Date: Wed, 24 Nov 2021 09:01:34 -0500 Message-ID: Subject: Re: [PATCH] net/memif: allow stopping and closing device To: Ferruh Yigit Cc: Junxiao Shi , dev@dpdk.org, "Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco)" Content-Type: multipart/alternative; boundary="0000000000004fdb5605d1894c61" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --0000000000004fdb5605d1894c61 Content-Type: text/plain; charset="UTF-8" Hi Ferruh You have to rely on user to call stop before calling close/remove. This is mandated in ethdev library, as implemented in: febc855b358e ("ethdev: forbid closing started device") Yours, Junxiao On Wed, Nov 24, 2021, 06:02 Ferruh Yigit wrote: > On 11/18/2021 5:33 PM, Junxiao Shi wrote: > > Bugzilla ID: 888 > > Fixes: febc855b358e ("ethdev: forbid closing started device") > > > > Signed-off-by: Junxiao Shi > > Thanks Junxiao, +1 to this fix, cc'ed memif maintainer Jakub. > > > --- > > drivers/net/memif/rte_eth_memif.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/memif/rte_eth_memif.c > b/drivers/net/memif/rte_eth_memif.c > > index 43d7378329..e3d523af57 100644 > > --- a/drivers/net/memif/rte_eth_memif.c > > +++ b/drivers/net/memif/rte_eth_memif.c > > @@ -1260,6 +1260,13 @@ memif_dev_start(struct rte_eth_dev *dev) > > return ret; > > } > > > > +static int > > +memif_dev_stop(struct rte_eth_dev *dev) > > +{ > > + memif_disconnect(dev); > > Is the 'memif_dev_stop()' safe to be called multiple times? > If 'memif_dev_close()' calls 'memif_dev_stop()' (see below), it is possible > to call 'memif_dev_stop()' multiple times, so it should be protected. > > > + return 0; > > +} > > + > > static int > > memif_dev_close(struct rte_eth_dev *dev) > > { > > @@ -1268,7 +1275,6 @@ memif_dev_close(struct rte_eth_dev *dev) > > > > if (rte_eal_process_type() == RTE_PROC_PRIMARY) { > > memif_msg_enq_disconnect(pmd->cc, "Device closed", 0); > > - memif_disconnect(dev); > > > > for (i = 0; i < dev->data->nb_rx_queues; i++) > > (*dev->dev_ops->rx_queue_release)(dev, i); > > @@ -1276,8 +1282,6 @@ memif_dev_close(struct rte_eth_dev *dev) > > (*dev->dev_ops->tx_queue_release)(dev, i); > > > > memif_socket_remove_device(dev); > > - } else { > > - memif_disconnect(dev); > > Should we add 'memif_dev_stop()' within the close function? > Otherwise we are relying on user to stop, but at least in remove path > ('rte_pmd_memif_remove()') that may not be the case. > > > } > > > > rte_free(dev->process_private); > > @@ -1515,6 +1519,7 @@ memif_rx_queue_intr_disable(struct rte_eth_dev > *dev, uint16_t qid __rte_unused) > > > > static const struct eth_dev_ops ops = { > > .dev_start = memif_dev_start, > > + .dev_stop = memif_dev_stop, > > .dev_close = memif_dev_close, > > .dev_infos_get = memif_dev_info, > > .dev_configure = memif_dev_configure, > > > > --0000000000004fdb5605d1894c61 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0Ferruh

You have to rely on user to call stop before calling close/remove= .
This is mandated in ethdev library, as implemented= in:
febc855b358e ("ethdev: forbid closing star= ted device")

Yours, Junxiao
On Wed, = Nov 24, 2021, 06:02 Ferruh Yigit <ferruh.yigit@intel.com> wrote:
On 11/18/2021 5:33 PM, Junxiao Shi wrote:
> Bugzilla ID: 888
> Fixes: febc855b358e ("ethdev: forbid closing started device"= )
>
> Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>
Thanks Junxiao, +1 to this fix, cc'ed memif maintainer Jakub.

> ---
>=C2=A0 =C2=A0drivers/net/memif/rte_eth_memif.c | 11 ++++++++---
>=C2=A0 =C2=A01 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte= _eth_memif.c
> index 43d7378329..e3d523af57 100644
> --- a/drivers/net/memif/rte_eth_memif.c
> +++ b/drivers/net/memif/rte_eth_memif.c
> @@ -1260,6 +1260,13 @@ memif_dev_start(struct rte_eth_dev *dev)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
>=C2=A0 =C2=A0}
>=C2=A0 =C2=A0
> +static int
> +memif_dev_stop(struct rte_eth_dev *dev)
> +{
> +=C2=A0 =C2=A0 =C2=A0memif_disconnect(dev);

Is the 'memif_dev_stop()' safe to be called multiple times?
If 'memif_dev_close()' calls 'memif_dev_stop()' (see below)= , it is possible
to call 'memif_dev_stop()' multiple times, so it should be protecte= d.

> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}
> +
>=C2=A0 =C2=A0static int
>=C2=A0 =C2=A0memif_dev_close(struct rte_eth_dev *dev)
>=C2=A0 =C2=A0{
> @@ -1268,7 +1275,6 @@ memif_dev_close(struct rte_eth_dev *dev)
>=C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (rte_eal_process_type() =3D=3D RTE_PROC_P= RIMARY) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memif_msg_enq_di= sconnect(pmd->cc, "Device closed", 0);
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memif_disconnect(dev)= ;
>=C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i = < dev->data->nb_rx_queues; i++)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0(*dev->dev_ops->rx_queue_release)(dev, i);
> @@ -1276,8 +1282,6 @@ memif_dev_close(struct rte_eth_dev *dev)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0(*dev->dev_ops->tx_queue_release)(dev, i);
>=C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memif_socket_rem= ove_device(dev);
> -=C2=A0 =C2=A0 =C2=A0} else {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memif_disconnect(dev)= ;

Should we add 'memif_dev_stop()' within the close function?
Otherwise we are relying on user to stop, but at least in remove path
('rte_pmd_memif_remove()') that may not be the case.

>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0rte_free(dev->process_private);
> @@ -1515,6 +1519,7 @@ memif_rx_queue_intr_disable(struct rte_eth_dev *= dev, uint16_t qid __rte_unused)
>=C2=A0 =C2=A0
>=C2=A0 =C2=A0static const struct eth_dev_ops ops =3D {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0.dev_start =3D memif_dev_start,
> +=C2=A0 =C2=A0 =C2=A0.dev_stop =3D memif_dev_stop,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0.dev_close =3D memif_dev_close,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0.dev_infos_get =3D memif_dev_info,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0.dev_configure =3D memif_dev_configure,
>

--0000000000004fdb5605d1894c61--