From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by dpdk.org (Postfix) with ESMTP id 8270791 for ; Wed, 31 Oct 2018 18:46:45 +0100 (CET) Received: by mail-wr1-f65.google.com with SMTP id 74-v6so9258779wrb.13 for ; Wed, 31 Oct 2018 10:46:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:content-transfer-encoding:mime-version; bh=seuXxw1D01geCvXis3mLPF+5u8f0bHNxtVwCQfAIoh8=; b=hDcuTl1sTq8Zp/pRzzAp+sDAF9+/oSmIv+e94gpo4/KpFaKIfvoVqM2HoOpEhAhflG A104whgztMsydNZAc9l6OLkBTwqf+h1oUYhI9S/zHgeCIGijfw1QudCwH+Vwk5y0Uxa+ fOvslXfFPJGOZu+GFPE83bi/EeWM6ovZnbsvBJgpBFf/IljLxe/hrwR6zUIS8DLRaL0M QqCJuOPOwr5/fUeE2HQUouej7QTxcdYGDc+gZqq+K89+SRaN38GEpBSPRes+6VSworcO 7LHM2/Wjoq77iQYiH5ogzCPqG2mV7e+vrlwXlvjUgMDqZT/HBr1P5B9D/q4ccRygDysr 8dAQ== X-Gm-Message-State: AGRZ1gIHC/uQ1fYtuaxu3ICaZZcpbORbg0nx9gAozcKH/ESjhsiZoIlr Agr6TxGr2NLnN3UuOBwLOn8= X-Google-Smtp-Source: AJdET5fgADZ8LX0mm7i5UCjeUbw5fyB3hlKa6xpsxILQeKFf06NDTWqrQ9Zp7YZmlXp4RnNndiK/kA== X-Received: by 2002:a5d:4747:: with SMTP id o7-v6mr3548027wrs.256.1541008004864; Wed, 31 Oct 2018 10:46:44 -0700 (PDT) Received: from localhost ([2a01:4b00:f419:6f00:8361:8946:ba2b:d556]) by smtp.gmail.com with ESMTPSA id f9-v6sm2021005wmb.8.2018.10.31.10.46.43 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 31 Oct 2018 10:46:43 -0700 (PDT) Message-ID: <1541008002.29722.34.camel@debian.org> From: Luca Boccassi To: Thomas Monjalon , llouis@vmware.com, yongwang@vmware.com Cc: dev@dpdk.org, Chas Williams <3chas3@gmail.com>, Maxime Coquelin , tiwei.bie@intel.com, Bruce Richardson , Jianfeng Tan , "Burakov, Anatoly" , brussell@vyatta.att-mail.com Date: Wed, 31 Oct 2018 17:46:42 +0000 In-Reply-To: <16755296.DkulTLluNz@xps> References: <20180816135032.28283-1-bluca@debian.org> <2858283.f32l3dFHSD@xps> <16755296.DkulTLluNz@xps> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.22.6-1+deb9u1 Mime-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 2/3] net/vmxnet3: fix vmxnet3 dev_uninit() hot-unplug 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: Wed, 31 Oct 2018 17:46:45 -0000 Sorry, been otherwise busy - I can do what you and Chas have asked, but the problem is that v1 already did that and the VMWare maintainers asked to change it back. So can I assume that the v1 way is the way to go? On Wed, 2018-10-31 at 18:27 +0100, Thomas Monjalon wrote: > Any update or question for this patch? > If no update, it will miss 18.11. >=20 >=20 > 27/10/2018 17:09, Thomas Monjalon: > > 19/09/2018 17:47, Chas Williams: > > > On Wed, Sep 19, 2018 at 8:58 AM Luca Boccassi > > > wrote: > > > >=20 > > > > The vmxnet3 driver can't call back into dev_close(), and > > > > possibly > > > > dev_stop(), in dev_uninit().=C2=A0=C2=A0When dev_uninit() is called= , > > > > anything > > > > that those routines would want to clean up has already been > > > > released. > > > > Further, for complete cleanup, it is necessary to release any > > > > of the > > > > queue resources during dev_close(). > > > > This allows a vmxnet3 device to be hot-unplugged without > > > > leaking > > > > queues. > > > >=20 > > > > Fixes: dfaff37fc46d ("vmxnet3: import new vmxnet3 poll mode > > > > driver implementation") > > > > Cc: stable@dpdk.org > > > >=20 > > > > Signed-off-by: Brian Russell > > > > Signed-off-by: Luca Boccassi > > > > --- > > > > v2: add back extra close() call in uninit() for buggy > > > > applications as > > > > =C2=A0=C2=A0=C2=A0=C2=A0requested by the reviewers, and add debug l= og noting the > > > > issue > > > >=20 > > > > =C2=A0drivers/net/vmxnet3/vmxnet3_ethdev.c | 35 > > > > +++++++++++++++++++++++----- > > > > =C2=A01 file changed, 29 insertions(+), 6 deletions(-) > > > >=20 > > > > diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > b/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > index f1596ab19d..98e5d01890 100644 > > > > --- a/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > +++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c > > > > @@ -354,8 +354,10 @@ eth_vmxnet3_dev_uninit(struct rte_eth_dev > > > > *eth_dev) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (rte_eal_process= _type() !=3D RTE_PROC_PRIMARY) > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return 0; > > >=20 > > > This should probably be EPERM as well.=C2=A0=C2=A0Out of scope though= . > > >=20 > > > >=20 > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (hw->adapter_stopped = =3D=3D 0) > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (hw->adapter_stopped = =3D=3D 0) { > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0PMD_INIT_LOG(DEBUG, "Device has not been > > > > closed."); > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmxnet3_dev_close(eth_dev); > > >=20 > > > This just seems wrong.=C2=A0=C2=A0You have called uninit() will the d= river > > > is > > > still busy.=C2=A0=C2=A0Instead of "fixing" the state of the driver, r= eturn > > > EBUSY > > > here. > >=20 > > I agree. > > If the port is not stopped, either you stop it or you return EBUSY. > >=20 > > Closing the device should be done outside of this check. > > It is OK to close from uninit if the app did not close it. > >=20 > > [...] > > > > +static void > > > > +vmxnet3_free_queues(struct rte_eth_dev *dev) > > > > +{ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int i; > > > > + > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0PMD_INIT_FUNC_TRACE(); > > > > + > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for (i =3D 0; i < dev->d= ata->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=C2=A0=C2=A0=C2=A0void *rxq =3D dev->data->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=C2=A0=C2=A0=C2=A0vmxnet3_dev_rx_queue_release(rxq); > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dev->data->nb_rx_queues = =3D 0; > > > > + > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for (i =3D 0; i < dev->d= ata->nb_tx_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=C2=A0=C2=A0=C2=A0void *txq =3D dev->data->tx_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=C2=A0=C2=A0=C2=A0vmxnet3_dev_tx_queue_release(txq); > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0dev->data->nb_tx_queues = =3D 0; > > > > =C2=A0} > > > >=20 > > > > =C2=A0/* > > > > @@ -844,12 +869,10 @@ vmxnet3_dev_stop(struct rte_eth_dev *dev) > > > > =C2=A0static void > > > > =C2=A0vmxnet3_dev_close(struct rte_eth_dev *dev) > > > > =C2=A0{ > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct vmxnet3_hw *hw = =3D dev->data->dev_private; > > > > - > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0PMD_INIT_FUNC_TRACE= (); > > > >=20 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmxnet3_dev_stop(de= v); > > > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0hw->adapter_stopped =3D = 1; > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0vmxnet3_free_queues(dev)= ; > > > > =C2=A0} > >=20 > > Good clean-up on dev_close. > > You probably want to go further and set RTE_ETH_DEV_CLOSE_REMOVE > > for allowing > > a real release of the port on close. > > Note: every PMDs should migrate towards this behaviour. > >=20 > > To make things clear (I will write a doc for -rc2): > > - "stop" should be called by the app but the PMD is allowed to > > force it. > > - "close" may be called by the app, and the PMD should enforce > > it in uninit. > > With RTE_ETH_DEV_CLOSE_REMOVE flag, it must completely > > release the port. > > - "remove" (implemented in PMD as uninit) is responsible of > > closing > > ethdev ports if not already done, and release the > > shared resources > > which are not specific to a port. It removes the whole > > EAL rte_device. > >=20 > > PS: for any hotplug patch or questions, feel free to Cc me. >=20 >=20 >=20 >=20 >=20 --=20 Kind regards, Luca Boccassi