Hi Ian, Thanks for your review. All your comments are addressed in v6 (to be sent shortly)
Regards, Ophir > -----Original Message----- > From: Stokes, Ian <[email protected]> > Sent: Tuesday, January 15, 2019 4:23 PM > To: Ophir Munk <[email protected]>; [email protected] > Cc: Asaf Penso <[email protected]>; Shahaf Shuler > <[email protected]>; Thomas Monjalon <[email protected]>; Olga > Shern <[email protected]>; Kevin Traynor <[email protected]>; Ilya > Maximets <[email protected]> > Subject: RE: [PATCH v5] 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]. > > > > Hi Ophir, thanks for the v5. > > A few comments below to be addressed. > > > +static int > > +netdev_dpdk_check_dup_dev(dpdk_port_t port_id, struct netdev *netdev, > > + const char *devargs, char **errp) { > > + struct netdev_dpdk *dup_dev; > > + > > + dup_dev = netdev_dpdk_lookup_by_port_id(port_id); > CLANG requires dpdk_mutex to be held above when calling > netdev_dpdk_lookup_by_port_id() above. > > lib/netdev-dpdk.c:1710:15: error: calling function > 'netdev_dpdk_lookup_by_port_id' requires holding mutex 'dpdk_mutex' > exclusively [-Werror,-Wthread-safety-analysis] > dup_dev = netdev_dpdk_lookup_by_port_id(port_id); > > In v6 netdev_dpdk_lookup_by_port_id() requires holding mutex dpdk_mutex > > +1846,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct > smap *args, > > /* Already configured, do not reconfigure again */ > > err = 0; > > } else { > > - struct netdev_dpdk *dup_dev; > > - > > - dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id); > > - if (dup_dev) { > > - VLOG_WARN_BUF(errp, "'%s' is trying to use device > > '%s' " > > - "which is already in use by > > '%s'", > > - netdev_get_name(netdev), new_devargs, > > - netdev_get_name(&dup_dev->up)); > > - err = EADDRINUSE; > > - } else { > > + err = netdev_dpdk_check_dup_dev(new_port_id, netdev, > > + new_devargs, errp); > > This introduces a second call to netdev_dpdk_check_dup_dev(), I thought that > this is already called as part of netdev_dpdk_process_devargs(), is this > second > call here required? When would this case hit? > > It seems to be specific to when the 'class=eth,mac=" devargs are passed, then > netdev_dpdk_check_dup_dev() won't be checked in > netdev_dpdk_process_devargs() and instead is checked here after > netdev_dpdk_process_devargs() returns a new_port_id that is not invalid and is > not the existing port id. > > I think a comment to clarify would be welcome here. > In v6 there is no more second checking of dup dev. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
