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

Trahe, Fiona fiona.trahe at intel.com
Mon Jan 8 15:51:51 CET 2018



> -----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 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