[dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id

[Wiles, Keith]

Sent from my iPhone

On Sep 26, 2016, at 9:24 AM, Iremonger, Bernard <bernard.iremonger at intel.com<mailto:bernard.iremonger at intel.com>> wrote:

Hi Bruce, Thomas,

<snip>

Subject: Re: [dpdk-dev] [PATCH] net/vhost: Add function to retreive the 'vid'
for a given port id

On Mon, Sep 26, 2016 at 04:26:27PM +0200, Thomas Monjalon wrote:
2016-09-26 14:18, Bruce Richardson:
On Mon, Sep 26, 2016 at 03:12:01PM +0200, Thomas Monjalon wrote:
2016-09-23 21:23, Wiles, Keith:
On Sep 23, 2016, at 12:26 AM, Yuanhan Liu
<yuanhan.liu at linux.intel.com<mailto:yuanhan.liu at linux.intel.com>> wrote:
On Thu, Sep 22, 2016 at 06:43:55PM +0200, Thomas Monjalon wrote:
There could be a similar need in other PMD.
If we can get an opaque identifier of the device which
is not the port id, we could call some specific
functions of the driver not implemented in the generic
ethdev API.

That means you have to add/export the PMD API first.
Isn't it against what you are proposing -- "I think we
should not add any API to the PMDs" ;)

Yes you are totally right :) Except that in vhost case,
we would not have any API in the PMD.
But it would allow to have some specific API in other
PMDs for the features which do not fit in a generic API.

So, does that mean you are okay with this patch now? I
mean, okay to introduce a vhost PMD API?

It means I would be in favor of introducing API in drivers
for very specific features.
In this case, I am not sure that retrieving an internal id is very
specific.

It's not, instead, it's very generic. The "internal id" is
actually the public interface to vhost-user application, like "fd" to
file APIs.

Instead of introducing a few specific wrappers/APIs, I'd
prefer to introduce a generic one to get the handle, and let
the application to call other vhost APIs.

Yes it makes sense.
I was thinking of introducing a function to get an internal
id from ethdev, in order to use it with any driver or underlying
library.
But it would be an opaque pointer and you need an int.
Note that we can cast an int into a pointer, so I am not sure what is
best.

Yes, that should work. But I just doubt what the "opaque
pointer" could be for other PMD drivers, and what the
application could do with it. For a typical nic PMD driver, I
can think of nothing is valuable to export to user applications.

But maybe it's valuable to other virtual PMD drives as well,
like the TAP pmd from Keith?

I do not see a need in the TAP PMD other then returning the FD for
TUN/TAP device. Not sure what any application would need with the FD
here, as it could cause some problems.

This feels like we are talking about a IOCTL like generic interface into
the PMD. Then we can add new one types and reject types in the PMD that
are not supported. Would this not be a better method for all future PMD
APIs?

Here is just a thought as to how to solve this problem without a PMD
specific API. A number of current ethdev APIs could be removed to use the
API below. The APIs would be removed from ethdev structure and have the
current APIs use the API below. I know some are not happy with number of
APIs in the ethdev structure.

The API could be something like this:
struct rte_tlv {        /* Type/Length/Value like structure */
   uint16_t type;    /* Type of command */
   uint16_t len;         /* Length of data section on input and on output
*/
   uint16_t tlen;        /* Total or max length of data buffer */
   uint8_t data[0];
};

int rte_eth_dev_ioctl(int pid, int qid, struct rte_tlv *tlv);

Yes we are talking about having some specific functions per driver
which are not defined in the generic ethdev layer.
We need only one function in ethdev to give access to driver-specific
API.
My idea is to convert the port id into an opaque handler.
Your idea is to use the port id in an ioctl like function.

Having a driver specific value is fine by me instead of using port id. But how do we get  the specific PMD id value?

I think it is a good idea to have something other then port id.


About the implementation, these are the 2 differences between my
proposal and yours:
- You use the well known port id, whereas I need another handler
which is understood by the driver.
- You need to build a message string which will be decoded by the
driver.
I propose to directly offer some specific functions in the drivers
which are more convenient to use and easier for code review/debug.

I think you want a set of apis specific to the driver, but this means everyone will have a different one or could be different.

I know Bruce does not like the generic function approach, but to get some type of standard using a generic function like the one I suggested is the only way I see it can work in the long run.

The type or command word could be a 32 bit value and the first two bytes use a pmd specific value or one of the common values we define.

Say 0x0000 is special set of common one we define and the pmd like TAP could use say 'T' << 16 | 'p' to create the upper 16 bit value that is special to the TAP driver. Not marriaged to the idea just one way to have specific and generic names spaces in the command word.


No conclusion here. I just want to make sure that we are on the
same page, and would like to have feedback from others. Thanks

I personally don't like the idea of having a generic IOCTL in
ethdev. If you want to have NIC-specific functions provided by a
driver, that is fine, but any app using those is going to be limited to
working only with that driver.

In that case, since the driver in question is known, I don't see any
reason to go through the ethdev layer. I think it would be much
clearer to have the app instead include the driver's header file and
call the driver function directly. The #include at the top of the
file makes the dependency very clear, and having a function name
instead of IOCTL with magic command numbers allows the action take by
the function to be clearer too.

So you are against an IOCTL API. Me too.
You agree that an application can be NIC-specific and include an
header file given by the driver to offer very specific features. Me too.

My proposal was to convert the port id to an opaque pointer as handler
of these driver APIs. After an offline discussion, we agreed that it
is not necessary because drivers manage rte_eth_dev struct and port_id
through
lib/librte_ether/rte_ethdev.h: extern struct rte_eth_dev
rte_eth_devices[];


+1. I agree with your proposal, and I also agree that no ethdev changes
+are
necessary to support drivers having their own private functions.

I am not sure what has been agreed here.

Looking at the code in struct rte_eth_dev{}

struct rte_eth_dev{
...
const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
...
}

The driver functions are only accessible if they are in struct eth_dev_ops.
I thought the issue here was that driver functions should not be added  to the struct eth_dev_ops.
Hence the need for an eth_dev API to return a pointer to a driver dev_ops structure containing the driver functions.

Regards,

Bernard.