From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f170.google.com (mail-pd0-f170.google.com [209.85.192.170]) by dpdk.org (Postfix) with ESMTP id AB44A5690 for ; Mon, 16 Mar 2015 09:38:38 +0100 (CET) Received: by pdbni2 with SMTP id ni2so52245984pdb.1 for ; Mon, 16 Mar 2015 01:38:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=5KZ298wGrHx/tJbja7AxGSr6x2lTgZbvpElgYiWrCiA=; b=Ik10P1ayVIaEBwXmvNFxyXPRsNJTRV1HMrajmsNSG6GOfhFjoj56aLXIiOkx2xTSD+ rhMnNXd0P/y+fkLDiihNXogW9kPe0bsaOLkSEjtJiz31VKcaM9M7NG/R1imhV+b3o84Y Z6ZAMZXND/jQatpvkkADhSvdZ1rj66nL+/1li/Ggd/KNNsO6J0HQcBHBBT0cA8dxi4CC 1fxKdor+uJ5UORdXwiY50hQtqssd5Iez8E+gfQj0Q6VzeRYwHq/nmBXgLq8CI0ccufvF eZVqUdOBtBjS1tVdK6gZ9p4j2lYaFrtCTBbo7RpfLDfT1m9HfDgWxSxIeSUddLMFEZ57 6kZQ== X-Gm-Message-State: ALoCoQmnsKDHVGtoq0UuTfvhUMLim/EjAX410k2JVNkArU87JIgASmEJX7y64Tf6drvLUO3TUE46 X-Received: by 10.70.101.137 with SMTP id fg9mr93867232pdb.101.1426495117777; Mon, 16 Mar 2015 01:38:37 -0700 (PDT) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id s17sm15982440pdn.87.2015.03.16.01.38.35 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Mar 2015 01:38:37 -0700 (PDT) Message-ID: <5506968C.4000908@igel.co.jp> Date: Mon, 16 Mar 2015 17:38:36 +0900 From: Tetsuya Mukawa User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: "Iremonger, Bernard" References: <1426012584-3614-1-git-send-email-linville@tuxdriver.com> <8CEF83825BEC744B83065625E567D7C2049F26C7@IRSMSX108.ger.corp.intel.com> <8CEF83825BEC744B83065625E567D7C2049F282A@IRSMSX108.ger.corp.intel.com> In-Reply-To: <8CEF83825BEC744B83065625E567D7C2049F282A@IRSMSX108.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Mar 2015 08:38:39 -0000 On 2015/03/13 19:14, Iremonger, Bernard wrote: > >> -----Original Message----- >> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp] >> Sent: Friday, March 13, 2015 12:11 AM >> To: Iremonger, Bernard >> Cc: John W. Linville; dev@dpdk.org >> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug >> >> 2015-03-13 2:05 GMT+09:00 Iremonger, Bernard : >>> >>>> -----Original Message----- >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of John W. Linvill= e >>>> Sent: Tuesday, March 10, 2015 6:36 PM >>>> To: dev@dpdk.org >>>> Subject: [dpdk-dev] [RFC] af_packet: support port hotplug >>>> >>>> This patch adds finalization code to free resources allocated by the= >>>> PMD. This is based on earlier patches for other PMDs by Tetsuya >>>> Mukawa , with corrections related to data- >>>>> name. >>>> Signed-off-by: John W. Linville >>>> Cc: Tetsuya Mukawa >>>> --- >>>> lib/librte_pmd_af_packet/rte_eth_af_packet.c | 56 >>>> ++++++++++++++++++++++++++++ >>>> 1 file changed, 56 insertions(+) >>>> >>>> diff --git a/lib/librte_pmd_af_packet/rte_eth_af_packet.c >>>> b/lib/librte_pmd_af_packet/rte_eth_af_packet.c >>>> index 80e9bdf7f819..57998ab19c96 100644 >>>> --- a/lib/librte_pmd_af_packet/rte_eth_af_packet.c >>>> +++ b/lib/librte_pmd_af_packet/rte_eth_af_packet.c >>>> @@ -399,6 +399,13 @@ static struct eth_dev_ops ops =3D { >>>> .stats_reset =3D eth_stats_reset, }; >>>> >>>> +static struct eth_driver rte_af_packet_pmd =3D { >>>> + .pci_drv =3D { >>>> + .name =3D "rte_af_packet_pmd", >>>> + .drv_flags =3D RTE_PCI_DRV_DETACHABLE, >>>> + }, >>>> +}; >>>> + >>>> /* >>>> * Opens an AF_PACKET socket >>>> */ >>>> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name, >>>> if (*eth_dev =3D=3D NULL) >>>> goto error; >>>> >>>> + /* check length of device name */ >>>> + if ((strlen((*eth_dev)->data->name) + 1) > sizeof(data->name))= >>>> + goto error; >>>> + >>>> /* >>>> * now put it all together >>>> * - store queue data in internals, @@ -669,12 +680,14 @@ >>>> rte_pmd_init_internals(const char *name, >>>> data->nb_tx_queues =3D (uint16_t)nb_queues; >>>> data->dev_link =3D pmd_link; >>>> data->mac_addrs =3D &(*internals)->eth_addr; >>>> + strncpy(data->name, (*eth_dev)->data->name, >>>> +strlen((*eth_dev)->data->name)); >>>> >>>> pci_dev->numa_node =3D numa_node; >>>> >>>> (*eth_dev)->data =3D data; >>>> (*eth_dev)->dev_ops =3D &ops; >>>> (*eth_dev)->pci_dev =3D pci_dev; >>>> + (*eth_dev)->driver =3D &rte_af_packet_pmd; >>>> >>>> return 0; >>>> >>>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, co= nst char *params) >>>> return 0; >>>> } >>>> >>>> +static int >>>> +rte_pmd_af_packet_devuninit(const char *name) { >>>> + struct rte_eth_dev *eth_dev =3D NULL; >>>> + struct pmd_internals *internals; >>>> + struct tpacket_req req; >>>> + >>>> + unsigned q; >>>> + >>>> + RTE_LOG(INFO, PMD, "Closing AF_PACKET ethdev on numa socket %u= \n", >>>> + rte_socket_id()); >>>> + >>>> + if (name =3D=3D NULL) >>>> + return -1; >>> Hi Tetsuya, John, >>> >>> Before detaching a port, the port must be stopped and closed. >>> The stop and close are only allowed for RTE_PROC_PRIMARY. >>> Should there be a check for process_type here? >>> >>> if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) >>> return -EPERM; >>> >>> Regards, >>> >>> Bernard >>> >> Hi Bernard, >> >> I agree with stop() and close() are only called by primary process, bu= t it may not need to add like >> above. >> Could you please check rte_ethdev.c? >> >> - struct rte_eth_dev_data *rte_eth_dev_data; This array is shared betw= een processes. >> So we need to initialize of finalize carefully like you said. >> >> - struct rte_eth_dev rte_eth_devices[] >> This array is per process. >> And 'data' variable of this structure indicates a pointer of rte_eth_d= ev_data. >> >> All PMDs for physical NIC allocates like above when PMDs are initializ= ed. >> (Even when a process is secondary, initialization function of PMDs wil= l be called) But virtual device >> PMDs allocate rte_eth_dev_data and overwrite 'data' >> variable of rte_eth_devices while initialization. >> >> As a result, primary and secondary process has their own 'rte_eth_dev_= data' for a virtual device. >> So I guess all processes need to free it not to leak memory. >> >> Thanks, >> Tetsuya >> > Hi Tetsuya, > > In rte_ethdev.c both rte_eth_dev_stop() and rte_eth_dev_close() use = the macro PROC_PRIMARY_OR_RET(). > So for secondary processes both functions return without doing anythin= g.=20 > Maybe this check should be added to rte_eth_dev_attach() and rte_eth_de= v_detach() ? > > For the Physical/Virtual Functions of the NIC a lot of the finalizati= on is done in the dev->dev_ops->dev_stop() and > dev->dev_ops->dev_close() functions. To complete the finalization the d= ev_uninit() function is called, this should probably do nothing for secon= dary processes as the dev_stop() and dev_close() functions will not have= been executed. Hi Bernard, Sorry for my English. I meant 'virtual device PMD' as PMDs like pcap or af_packet PMDs. Not a PMDs for virtual functions on NIC. For PMDs like a pcap and af_packet PMDs, all data structures are allocated per processes. (Actually I guess nothing is shared between primary and secondary processes, because rte_eth_dev_data is overwritten by each processes.) So we need to free per process data when detach() is called. > For the Physical/Virtual Functions of the NIC the dev_init() is calle= d for both primary and secondary processes, however a subset of the funct= ion only is executed for secondary processes. Because of above, we probably not be able to add PROC_PRIMARY_OR_RET() to rte_eth_dev_detach(). But I agree we should not call rte_eth_dev_detach() for secondary process, if PMDs are like e1000 or ixgbe PMD. To work like above, how about changing drv_flags dynamically in close() callback? For example, when primary process calls rte_eth_dev_close(), a callback of PMD will be called. (In the case of e1000 PMD, eth_em_close() is the callback.) At that time, specify RTE_PCI_DRV_DETACHABLE flag to drv_flag in the callback. It means if primary process hasn't called close() yet, rte_eth_dev_detach() will do nothing and return error. How about doing like above? Regards, Tetsuya > Regards, > > Bernard. > =20 > > > > > >