Hi Ian,
Thank you for your review.
Please see comments inline.
> -----Original Message-----
> From: Ian Stokes <[email protected]>
> Sent: Friday, January 4, 2019 8:55 PM
> To: Ophir Munk <[email protected]>; [email protected]
> Cc: Asaf Penso <[email protected]>; Shahaf Shuler
> <[email protected]>; Kevin Traynor <[email protected]>; Thomas
> Monjalon <[email protected]>; Olga Shern <[email protected]>
> Subject: Re: [PATCH v3] netdev-dpdk: support port representors
>
> > Dpdk port representors were introduced in dpdk versions 18.xx.
> > Prior to port representors there was a one-to-one relationship > between
> an rte device (e.g. PCI bus) and an eth device (referenced as > dpdk port id
> in OVS). With port representors the relationship becomes > one-to-many rte
> device to eth devices.
> > For example in [3] there are two devices (representors) using the same >
> PCI physical address 0000:08:00.0: "0000:08:00.0,representor=[3]" and >
> "0000:08:00.0,representor=[5]".
> > This commit handles the new one-to-many relationship. For example, >
> when one of the device port representors in [3] is closed - the PCI bus >
> cannot be detached until the other device port representor is closed as >
> well. OVS remains backward compatible by supporting dpdk legacy PCI >
> ports which do not include port representors.
> > Dpdk port representors related commits are listed in [1]. Dpdk port >
> representors documentation appears in [2]. A sample configuration > which
> uses two representors ports (the output of "ovs-vsctl show"
> > command) is shown in [3].
>
> Thanks for the v3 Ophir, a few comments inline below.
>
....
> In terms of testing I've tested with a number of devices (140e, ixgbe, igb and
> associated VFs). I didn't have a device that was multiple port to single bus
> slot
> function address unfortunately, hopefully someone on the thread could
> verify that case.
>
> I'd like to see a fix me comment added here also as we're accessing the
> rte_eth_devices array directly.
>
FIXME will be added in v4.
> > + rte_dev = rte_eth_devices[dev->port_id].device;
> > + if (netdev_dpdk_get_num_ports(rte_dev) == 1) {
> > + if (rte_dev_remove(rte_dev) < 0) {
> > + VLOG_ERR("Device '%s' can not be detached",
> dev->devargs);
> > + }
> > }
> > }
> >
...
> > +/* Return the first DPDK port_id matching the devargs pattern. */
> > +static dpdk_port_t
> > +netdev_dpdk_get_port_by_devargs(const char *devargs)
> > +{
> > + struct rte_dev_iterator iterator;
> > + dpdk_port_t port_id;
> > +
> > + if (rte_dev_probe(devargs)) {
>
> When testing this I noticed the following error being reported when a
> port is added to OVS DPDK.
>
> dpdk|ERR|EAL: Failed to attach device on primary process
>
> Note the port was still successfully added to OVS DPDK but I was
> confused when reviewing the logs.
>
> In my case the error occurs because I had already bound the device in
> question to the igb_uio before I started OVS DPDK (rather than
> hotplugging the device after starting OVS DPDK).
>
> When OVS DPDK starts the device will be probed if it is bound to a
> userspace driver (e.g. igb_uio).
>
> Later when I add the port to OVS via ovs-vsctl it will be probed again
> because of the introduction of the call rte_dev_probe() above, which
> returns an EEXIST error, causing the DPDK ERR message above before then
> successfully adding the port to OVS DPDK.
>
> I think this behavior will cause some confusion for a users, the error
> message from DPDK is vague and unless you dive into the probe code it's
> not clear whats causing the issue or if there is a real issue here at all.
>
> Is it possible to see if a device has already been probed, and if so
> skip the rte_dev_probe(devargs) above?
Yes. I will implement your suggestion in v4.
...
> > @@ -3253,15 +3311,22 @@ netdev_dpdk_detach(struct unixctl_conn
> *conn,
> int argc OVS_UNUSED,
> > goto error;
> > }
> >
> > - rte_eth_dev_close(port_id);
>
> Same as above, Fix me comment for rte_eth_devices access.
FIXME will be added in v4.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
