Hello Fiona,

Apologies for delay in my response.
Some comments inline...

On Monday 08 January 2018 08:21 PM, Trahe, Fiona wrote:

quoted

-----Original Message-----
From: Shreyansh Jain [mailto:shreyansh.jain@nxp.com]
Sent: Monday, January 8, 2018 2:10 PM
To: Trahe, Fiona <redacted>
Cc: Hemant Agrawal <redacted>; Xu, Rosen <redacted>; 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:

quoted

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)

[...]

quoted

quoted

+#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.

quoted

[...]

quoted

quoted

+
+/**
+ * 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.

quoted

quoted

+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

[...]

quoted

quoted

+
+/**
+ * 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

quoted

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

quoted

The application wants to look for any or all xyz devices so must know the naming format used by the

PMD.

quoted

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?

quoted

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