[dpdk-dev] [PATCH v1 1/5] rawdev: introduce raw device library support

Shreyansh Jain shreyansh.jain at nxp.com
Fri Jan 12 16:00:54 CET 2018


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 at nxp.com]
>> Sent: Monday, January 8, 2018 2:10 PM
>> To: Trahe, Fiona <fiona.trahe at intel.com>
>> Cc: Hemant Agrawal <hemant.agrawal at nxp.com>; Xu, Rosen <rosen.xu at intel.com>; dev at 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



More information about the dev mailing list