From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v1 1/5] rawdev: introduce raw device library support Date: Fri, 12 Jan 2018 20:30:54 +0530 Message-ID: <4adc1036-6d91-a550-a80a-37347dae184c@nxp.com> References: <20180102125749.2379-1-shreyansh.jain@nxp.com> <20180102125749.2379-2-shreyansh.jain@nxp.com> <348A99DA5F5B7549AA880327E580B435892EFE92@IRSMSX101.ger.corp.intel.com> <348A99DA5F5B7549AA880327E580B435892F0D40@IRSMSX101.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: Hemant Agrawal , "Xu, Rosen" , "dev@dpdk.org" To: "Trahe, Fiona" Return-path: Received: from NAM01-SN1-obe.outbound.protection.outlook.com (mail-sn1nam01on0059.outbound.protection.outlook.com [104.47.32.59]) by dpdk.org (Postfix) with ESMTP id 3815F1B011 for ; Fri, 12 Jan 2018 15:46:42 +0100 (CET) In-Reply-To: <348A99DA5F5B7549AA880327E580B435892F0D40@IRSMSX101.ger.corp.intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Fiona, Apologies for delay in my response. Some comments inline... On Monday 08 January 2018 08:21 PM, Trahe, Fiona wrote: > > >> -----Original Message----- >> From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com] >> Sent: Monday, January 8, 2018 2:10 PM >> To: Trahe, Fiona >> Cc: Hemant Agrawal ; Xu, Rosen ; dev@dpdk.org >> Subject: Re: [PATCH v1 1/5] rawdev: introduce raw device library support >> >> Hello Fiona, >> >> On Saturday 06 January 2018 07:10 PM, Trahe, Fiona wrote: >>> Hi Shreyansh, >>> >>> This looks like a useful generic device, thanks. Some comments below. >> >> Thanks for taking interest and sending your review. >> I have some responses inline.... >> (And I have shortened the original email) >> >> [...] >> >>>> +#include "rte_rawdev.h" >>>> +#include "rte_rawdev_pmd.h" >>>> + >>>> +/* dynamic log identifier */ >>>> +int librawdev_logtype; >>>> + >>>> +/* Maximum rawdevices supported by system. >>>> + */ >>>> +#define RTE_MAX_RAWDEVPORTS 10 >>> [Fiona] Typo in comment above? There's RTE_RAWDEV_MAX_DEVS, RTE_MAX_RAWDEVS and >> RTE_MAX_RAWDEVPORTS. Are all 3 necessary and what's the relationship between ports and devs? >> >> This is a stupid mistake by me. It should be only RTE_RAWDEV_MAX_DEVS. >> RTE_MAX_RAWDEVS is useless and I will remove RTE_MAX_RAWDEVPORTS. >> They are intend the same thing - number of max devices supported. >> >>> >> >> [...] >> >>>> + >>>> +/** >>>> + * Allocate and set up a raw queue for a raw device. >>>> + * >>>> + * @param dev_id >>>> + * The identifier of the device. >>>> + * @param queue_id >>>> + * The index of the raw queue to setup. The value must be in the range >>>> + * [0, nb_raw_queues - 1] previously supplied to rte_rawdev_configure(). >>>> + * >>>> + * @see rte_rawdev_queue_conf_get() >>>> + * >>>> + * @return >>>> + * - 0: Success, raw queue correctly set up. >>>> + * - <0: raw queue configuration failed >>>> + */ >>> [Fiona] cut and paste error above - should be release. >> >> Indeed. Thanks for pointing out. >> I will fix this. >> >>> >>>> +int >>>> +rte_rawdev_queue_release(uint16_t dev_id, uint16_t queue_id); >>>> +/** >>>> + * Get the number of raw queues on a specific raw device >>>> + * >>>> + * @param dev_id >>>> + * Raw device identifier. >>>> + * @return >>>> + * - The number of configured raw queues >>>> + */ >>>> +uint16_t >> >> [...] >> >>>> + >>>> +/** >>>> + * Allocates a new rawdev slot for an raw device and returns the pointer >>>> + * to that slot for the driver to use. >>>> + * >>>> + * @param name >>>> + * Unique identifier name for each device >>>> + * @dev_priv_size >>>> + * Private data allocated within rte_rawdev object. >>>> + * @param socket_id >>>> + * Socket to allocate resources on. >>>> + * @return >>>> + * - Slot in the rte_dev_devices array for a new device; >>>> + */ >>>> +struct rte_rawdev * >>>> +rte_rawdev_pmd_allocate(const char *name, size_t dev_private_size, >>>> + int socket_id); >>> [Fiona] The driver must allocate a unique name for each device, and the application presumably must >> search through all devices using dev_count and dev_info_get for each >>> until it finds a name it expects? But will the application always know the name chosen by the PMD? e.g. >> driver type xyz might find 10 devices and call them xyz_0, xyz_1, xyz_2, etc >>> The application wants to look for any or all xyz devices so must know the naming format used by the >> PMD. >>> Would it be useful to have 2 parts to the name, a type and an instance, to facilitate finding all devices of >> a specific type? >> >> let me state what I have understood: >> >> There are two types of devices: >> 1. which are scanned through a bus (PCI ...) >> 2. which are created through vdev (devargs, vdev_init) >> >> for those which are scanned through a bus, it is easy to append a >> "type_" string during device naming. >> for those which are added through command line, this pattern would have >> to be choosen by the application/user. >> >> further, a rawdevice doesn't have a specific type. So, type would be >> purely be defined by the driver (scan) or the device name itself >> (vdev_init). >> >> So, eventually the "type_" field would be left out for driver or >> application to decide. framework (lib/librte_rawdev) would never >> override/append to it. >> >> Is this understanding correct? > [Fiona] Yes. I'm probably overcomplicating it. > I was considering scanned devices and e.g. a case where 2 PMDs inadvertently pick the same name. > One idea would be each driver would register a type string with the lib layer and > all its device names must start with this, thus ensuring that each device name is unique. > With the vdev devices the application can ensure device names are unique. > A driver would not be allowed to use a name starting with a string which another PMD had already registered. > > This would allow looser coupling between the applications and the PMDs, as applications > would not need to know the exact name format of device name, just know the type it wants to use > and search for all devices with names starting with that string. > But I'm probably anticipating issues which wouldn't happen in real world applications. > i.e. though there may be many PMDs and applications in the dpdk codebase using this lib in future, > it's likely only a small number will be compiled into any build so such name clashes are unlikely to occur. > And the applications must be tightly coupled with the PMD anyway to make use of the device, so > that's probably not a concern either. I agree with the last line that applications have to be tightly coupled with PMD in this case. That defines (or prevents defining) a lot of semantics. While creating the patches, even I was thinking of standardizing the naming (taking hint from some of Gaetan's work on devargs) but I couldn't think of a policy which can be enforced only by the rawlib. I concur with you that conflicting naming in a real world scenario is theoretically possible, irrespective of how rare it might be. I suggest that we continue as is and expand this in future when we have more clarity or even some real-world application samples. You have any objections to this? > >> >> I will send a v2 shortly with your comments. I will also try and think >> through your suggestion about name containing "type_" - I do think it is >> useful but not really sure how would it define semantics between driver >> and application. >> >> - >> Shreyansh