From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f53.google.com (mail-pa0-f53.google.com [209.85.220.53]) by dpdk.org (Postfix) with ESMTP id DF6D25690 for ; Mon, 16 Mar 2015 09:56:53 +0100 (CET) Received: by pacwe9 with SMTP id we9so59015666pac.1 for ; Mon, 16 Mar 2015 01:56:53 -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=Nj3burCMs7UOXXL48dlibXq0Q1vl0YGweDTKPJyGzGM=; b=Ulvdd241sePk28wz4jEtbEvcjkOz+o0Ejx+z5gW6nfuybgZFOcelWwacTSvLU2/n2S Agi1efDDllGCHuMZPupglQkdQhrTfmGczpIyo3fR8PJ7O84ekh79GhqbfEX/cW/ijWxG 4DZXSX6q482ZcNvbsSdYi14lbgfwYYEGAzv84r2/Mi8l1qMtBA53lfPezV9rNHcU6kkt 16Ox9eP269Bcm1yToQTdmo0Ul+rH209w3YuNS06OpxVpyax6zKhM47vBHdlY2Q2n+q7l 0np/83SX9zpOM5ues+J98AFH8OvRp85YZKu9lKR88X/SfPLRsUf+2QmhuNpa1PPJ4b4n grdA== X-Gm-Message-State: ALoCoQm43N1Y3YIey26QMb9cCdsX7zD3qhQHc+S875kMpWy++1A7WREVaqloWPtFyWfq1ExmomUm X-Received: by 10.66.124.227 with SMTP id ml3mr136581724pab.28.1426496213134; Mon, 16 Mar 2015 01:56:53 -0700 (PDT) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id dl1sm16080607pdb.75.2015.03.16.01.56.51 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Mar 2015 01:56:52 -0700 (PDT) Message-ID: <55069AD4.6040202@igel.co.jp> Date: Mon, 16 Mar 2015 17:56:52 +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> <5506968C.4000908@igel.co.jp> In-Reply-To: <5506968C.4000908@igel.co.jp> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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:56:54 -0000 On 2015/03/16 17:38, Tetsuya Mukawa wrote: > 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. Linville >>>>> 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 = { >>>>> .stats_reset = eth_stats_reset, }; >>>>> >>>>> +static struct eth_driver rte_af_packet_pmd = { >>>>> + .pci_drv = { >>>>> + .name = "rte_af_packet_pmd", >>>>> + .drv_flags = RTE_PCI_DRV_DETACHABLE, >>>>> + }, >>>>> +}; >>>>> + >>>>> /* >>>>> * Opens an AF_PACKET socket >>>>> */ >>>>> @@ -653,6 +660,10 @@ rte_pmd_init_internals(const char *name, >>>>> if (*eth_dev == 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 = (uint16_t)nb_queues; >>>>> data->dev_link = pmd_link; >>>>> data->mac_addrs = &(*internals)->eth_addr; >>>>> + strncpy(data->name, (*eth_dev)->data->name, >>>>> +strlen((*eth_dev)->data->name)); >>>>> >>>>> pci_dev->numa_node = numa_node; >>>>> >>>>> (*eth_dev)->data = data; >>>>> (*eth_dev)->dev_ops = &ops; >>>>> (*eth_dev)->pci_dev = pci_dev; >>>>> + (*eth_dev)->driver = &rte_af_packet_pmd; >>>>> >>>>> return 0; >>>>> >>>>> @@ -835,10 +848,53 @@ rte_pmd_af_packet_devinit(const char *name, const char *params) >>>>> return 0; >>>>> } >>>>> >>>>> +static int >>>>> +rte_pmd_af_packet_devuninit(const char *name) { >>>>> + struct rte_eth_dev *eth_dev = 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 == 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() != RTE_PROC_PRIMARY) >>>> return -EPERM; >>>> >>>> Regards, >>>> >>>> Bernard >>>> >>> Hi Bernard, >>> >>> I agree with stop() and close() are only called by primary process, but 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 between 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_dev_data. >>> >>> All PMDs for physical NIC allocates like above when PMDs are initialized. >>> (Even when a process is secondary, initialization function of PMDs will 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 anything. >> Maybe this check should be added to rte_eth_dev_attach() and rte_eth_dev_detach() ? >> >> For the Physical/Virtual Functions of the NIC a lot of the finalization is done in the dev->dev_ops->dev_stop() and >> dev->dev_ops->dev_close() functions. To complete the finalization the dev_uninit() function is called, this should probably do nothing for secondary 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 called for both primary and secondary processes, however a subset of the function 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. Correction: We should not process rte_eth_dev_detach() for secondary process, if PMDs are like e1000 or ixgbe PMD and if primary process hasn't called stop() and close() yet. Tetsuya > > 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. >> >> >> >> >> >> >> >