From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f171.google.com (mail-pd0-f171.google.com [209.85.192.171]) by dpdk.org (Postfix) with ESMTP id D650A569C for ; Tue, 17 Mar 2015 04:42:49 +0100 (CET) Received: by pdbcz9 with SMTP id cz9so77188750pdb.3 for ; Mon, 16 Mar 2015 20:42:49 -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=FUc3iLB/q3U6O+6glLAFaOfQ5yVkvvbm/3EikRbu+uI=; b=DMx1ZKHMmEBgsRg3oqFZ5S0XfyfBRS8aFQegypAl+ygSBFC/rdkjwhHaeq1MiffJ+Y /ADQi8zPqoRskvs65MwhZ/zzLaebe6WnoANkwqG9VgUhUv0kxJUZhLEKY04pbaehlQhf /ekLkry+ECfTrO8ka9gaJIC+9tWoLfX2J+igmjohehs3FasGmXzXcVjDVP2zxAyUjnDZ KIQur9llyoP1uDtN3bEVLKymUcoEZ0sTpIoAtnBmWwdsSW4nrb30WAZgVkJ+ZgcZUEFm pmWWgNSj6tgzk05E5lpW/KrjryGsVckIBXLVaVIDQLH9ezJqmfIpIcQy3iJWLkJGgMvq 8T9w== X-Gm-Message-State: ALoCoQnw7LVf2ut3QjayqrsbcsDtdBWovYK4L7Uosc/aaVUSo+lk+XIXy8x5JHSkw7eUZ97NZu+z X-Received: by 10.70.56.34 with SMTP id x2mr144464588pdp.127.1426563768998; Mon, 16 Mar 2015 20:42:48 -0700 (PDT) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id q6sm19638033pdr.35.2015.03.16.20.42.47 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Mar 2015 20:42:48 -0700 (PDT) Message-ID: <5507A2B7.1040102@igel.co.jp> Date: Tue, 17 Mar 2015 12:42:47 +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> <55069AD4.6040202@igel.co.jp> <8CEF83825BEC744B83065625E567D7C2049F2FF4@IRSMSX108.ger.corp.intel.com> In-Reply-To: <8CEF83825BEC744B83065625E567D7C2049F2FF4@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: Tue, 17 Mar 2015 03:42:50 -0000 On 2015/03/16 23:47, Iremonger, Bernard wrote: > >> -----Original Message----- >> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp] >> Sent: Monday, March 16, 2015 8:57 AM >> To: Iremonger, Bernard >> Cc: John W. Linville; dev@dpdk.org >> Subject: Re: [dpdk-dev] [RFC] af_packet: support port hotplug >> >>>>>>> @@ -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 =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,= >>>>> 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 b= etween 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_et= h_dev_data. >>>>> >>>>> All PMDs for physical NIC allocates like above when PMDs are initia= lized. >>>>> (Even when a process is secondary, initialization function of PMDs >>>>> will be called) But virtual device PMDs allocate rte_eth_dev_data a= nd overwrite 'data' >>>>> variable of rte_eth_devices while initialization. >>>>> >>>>> As a result, primary and secondary process has their own 'rte_eth_d= ev_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() u= se the macro >> PROC_PRIMARY_OR_RET(). >>>> So for secondary processes both functions return without doing anyt= hing. >>>> 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 th= e dev_uninit() function is >> called, this should probably do nothing for secondary processes as th= e 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 ca= lled 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 P= MDs 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 > Hi Tetsuya, > For the e1000, igb and ixgbe PMD's it is probably simpler to just chec= k for the primary process in the uninit functions and just return without= doing anything for secondary processes. Thanks for clarifying. In the case, is it okay for you to add PROC_PRIMARY_OR_RET() in e1000, igb and ixgbe PMD code? If it's okay, we may be able to ACK this patch. :) Regards, Tetsuya > > Regards, > > Bernard. > > > > >