[ovs-dev] [dpdk-hwol PATCH v1] netdev-dpdk: support port representors
Ophir Munk
ophirmu at mellanox.com
Sat Dec 15 23:28:23 UTC 2018
Hi Aaron,
It seems like a false negative, nevertheless this error must be eliminated.
Please note that line 700:
dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
is called inside static function netdev_dpdk_process_devargs()
which is called only once by another static function netdev_dpdk_set_config() where the dpdk_mutex is taken.
So why does clang issue an error?
I guess that since function netdev_dpdk_process_devargs() did not take dpdk_mutes explicitly inside its body - clang cannot detect the transitivity from netdev_dpdk_set_config to netdev_dpdk_process_devrags of taking the dpdk_mutex
I suggest adding OVS_REQUIRES(dpdk_mutex) explicitly in netdev_dpdk_process_devrags() in a hope to fix this error.
I will add it in v2.
Is there a way to verify the clang errors independently before sending patches? Can you please advise on this?
Regards,
Ophir
1686 static dpdk_port_t
1687 netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
1688 const char *devargs, char **errp)
1689 OVS_REQUIRES(dpdk_mutex)
1690 {
1691 dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
1692
1693 if (strncmp(devargs, "class=eth,mac=", 14) == 0) {
1694 new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]);
1695 } else {
1696 new_port_id = netdev_dpdk_get_port_by_devargs(devargs);
1697 if (!rte_eth_dev_is_valid_port(new_port_id)) {
1698 new_port_id = DPDK_ETH_PORT_ID_INVALID;
1699 } else {
1700 struct netdev_dpdk *dup_dev;
1701
1702 dup_dev = netdev_dpdk_lookup_by_port_id(new_port_id);
1703 if (dup_dev) {
1704 VLOG_WARN_BUF(errp, "'%s' is trying to use d
static int
1755 netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
1756 char **errp)
1757 {
> -----Original Message-----
> From: Aaron Conole <aconole at bytheb.org>
> Sent: Sunday, December 16, 2018 12:12 AM
> To: Ophir Munk <ophirmu at mellanox.com>
> Cc: ovs-dev at openvswitch.org; Shahaf Shuler <shahafs at mellanox.com>;
> Thomas Monjalon <thomas at monjalon.net>
> Subject: Re: [ovs-dev] [dpdk-hwol PATCH v1] netdev-dpdk: support port
> representors
>
> Ophir Munk <ophirmu at mellanox.com> writes:
>
> > Dpdk port representors were introduced in dpdk 18.xx.
> > Prior to representors there was a one-to-one relationship between an
> > rte device (e.g. PCI bus) and an eth device (a dpdk port id). With
> > 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 representors in [3] is closed - the PCI bus
> > cannot be detached until the other device representor is closed as
> > well. OVS remains backward compatible by supporting dpdk legacy PCI
> > ports which do not include representors.
> > Dpdk representors related commits are listed in [1]. dpdk representors
> > documentation appears in [2]. A sample configuration which uses two
> > representors ports (the output of "ovs-vsctl show" command) is shown
> > in [3].
> >
> > [1]
> > e0cb96204b71 ("net/i40e: add support for representor ports")
> > cf80ba6e2038 ("net/ixgbe: add support for representor ports")
> > 26c08b979d26 ("net/mlx5: add port representor awareness")
> >
> > [2]
> > doc/guides/prog_guide/switch_representation.rst
> >
> > [3]
> > Bridge "ovs_br0"
> > Port "ovs_br0"
> > Interface "ovs_br0"
> > type: internal
> > Port "port-rep3"
> > Interface "port-rep3"
> > type: dpdk
> > options: {dpdk-devargs="0000:08:00.0,representor=[3]"}
> > Port "port-rep5"
> > Interface "port-rep5"
> > type: dpdk
> > options: {dpdk-devargs="0000:08:00.0,representor=[5]"}
> > ovs_version: "2.10.90"
> >
> > Signed-off-by: Ophir Munk <ophirmu at mellanox.com>
> > ---
>
> FYI, the following clang error seems to be spit out by this patch:
>
> libtool: compile: clang -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I
> ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -
> Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-
> function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -
> Wmissing-prototypes -Wmissing-field-initializers -Wthread-safety -fno-strict-
> aliasing -Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument -
> Wshift-negative-value -Qunused-arguments -Wshadow -Warray-bounds-
> pointer-arithmetic -mssse3 -I./dpdk-18.11/build/include -
> D_FILE_OFFSET_BITS=64 -Werror -Wno-cast-align -Wno-error=unused-
> command-line-argument -MT lib/stream-ssl.lo -MD -MP -MF
> lib/.deps/stream-ssl.Tpo -c lib/stream-ssl.c -o lib/stream-ssl.o
> lib/netdev-dpdk.c:1700:23: 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(new_port_id);
>
> > v1:
> > 1. rebase on top of Kevin's patch
> > dpdk: Update to use DPDK 18.11.[ovs-dev,v7,dpdk-latest,1/1] dpdk:
> Update to use DPDK 18.11.
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >
> chwork.ozlabs.org%2Fpatch%2F1005535%2F&data=02%7C01%7Cophirm
> u%40me
> >
> llanox.com%7Cf6a20b11663f4ab1a39608d662da4f79%7Ca652971c7d2e4d9ba6
> a4d1
> >
> 49256f461b%7C0%7C0%7C636805087113130623&sdata=A%2BiFB3q%2B
> HNqRPua0
> > qyCA06ZwfapeUNKNG2%2F0XsbNcRE%3D&reserved=0
> > 2. skipping count of sibling ports in case the sibling port state is
> > RTE_ETH_DEV_UNUSED
> >
> > lib/netdev-dpdk.c | 112
> > ++++++++++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 83 insertions(+), 29 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 6b8e05e..30af043 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -1216,6 +1216,25 @@ dpdk_dev_parse_name(const char dev_name[],
> const char prefix[],
> > }
> > }
> >
> > +/* get the number of OVS interfaces which have the same DPDK
> > + * rte device (e.g. same pci bus address). */ static int
> > +netdev_dpdk_get_num_ports(struct rte_device *device)
> > + OVS_REQUIRES(dpdk_mutex)
> > +{
> > + struct netdev_dpdk *dev;
> > + int count;
> > +
> > + count = 0;
> > + LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> > + if (rte_eth_devices[dev->port_id].device == device
> > + && rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED)
> {
> > + count++;
> > + }
> > + }
> > + return count;
> > +}
> > +
> > static int
> > vhost_common_construct(struct netdev *netdev)
> > OVS_REQUIRES(dpdk_mutex)
> > @@ -1351,20 +1370,23 @@ static void
> > netdev_dpdk_destruct(struct netdev *netdev) {
> > struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> > - struct rte_eth_dev_info dev_info;
> > + struct rte_device *rte_dev;
> >
> > ovs_mutex_lock(&dpdk_mutex);
> >
> > rte_eth_dev_stop(dev->port_id);
> > dev->started = false;
> > -
> > if (dev->attached) {
> > + /* Remove the port eth device */
> > rte_eth_dev_close(dev->port_id);
> > - rte_eth_dev_info_get(dev->port_id, &dev_info);
> > - if (dev_info.device && !rte_dev_remove(dev_info.device)) {
> > - VLOG_INFO("Device '%s' has been detached", dev->devargs);
> > - } else {
> > - VLOG_ERR("Device '%s' can not be detached", dev->devargs);
> > + VLOG_INFO("Device '%s' has been removed", dev->devargs);
> > + /* if this is the last port_id using this rte device
> > + * remove this rte device and all its eth devices */
> > + 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);
> > + }
> > }
> > }
> >
> > @@ -1630,8 +1652,26 @@ netdev_dpdk_get_port_by_mac(const char
> *mac_str)
> > return DPDK_ETH_PORT_ID_INVALID;
> > }
> >
> > +/* 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)) {
> > + port_id = DPDK_ETH_PORT_ID_INVALID;
> > + } else {
> > + RTE_ETH_FOREACH_MATCHING_DEV(port_id, devargs, &iterator) {
> > + break;
> > + }
> > + }
> > + return port_id;
> > +}
> > +
> > /*
> > - * Normally, a PCI id is enough for identifying a specific DPDK port.
> > + * Normally, a PCI id (optionally followed by a representor number)
> > + * is enough for identifying a specific DPDK port.
> > * However, for some NICs having multiple ports sharing the same PCI
> > * id, using PCI id won't work then.
> > *
> > @@ -1644,29 +1684,32 @@ static dpdk_port_t
> > netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
> > const char *devargs, char **errp) {
> > - char *name;
> > dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
> >
> > if (strncmp(devargs, "class=eth,mac=", 14) == 0) {
> > new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]);
> > } else {
> > - name = xmemdup0(devargs, strcspn(devargs, ","));
> > - if (rte_eth_dev_get_port_by_name(name, &new_port_id)
> > - || !rte_eth_dev_is_valid_port(new_port_id)) {
> > - /* Device not found in DPDK, attempt to attach it */
> > - if (!rte_dev_probe(devargs)
> > - && !rte_eth_dev_get_port_by_name(name, &new_port_id)) {
> > - /* Attach successful */
> > - dev->attached = true;
> > - VLOG_INFO("Device '%s' attached to DPDK", devargs);
> > - } else {
> > - /* Attach unsuccessful */
> > + new_port_id = netdev_dpdk_get_port_by_devargs(devargs);
> > + if (!rte_eth_dev_is_valid_port(new_port_id)) {
> > + new_port_id = DPDK_ETH_PORT_ID_INVALID;
> > + } 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(&dev->up), devargs,
> > + netdev_get_name(&dup_dev->up));
> > new_port_id = DPDK_ETH_PORT_ID_INVALID;
> > + } else {
> > + /* device successfully found */
> > + dev->attached = true;
> > + VLOG_INFO("Device '%s' attached to DPDK port %d",
> > + devargs, new_port_id);
> > }
> > }
> > - free(name);
> > }
> > -
> > if (new_port_id == DPDK_ETH_PORT_ID_INVALID) {
> > VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
> devargs);
> > }
> > @@ -3234,11 +3277,15 @@ netdev_dpdk_detach(struct unixctl_conn
> *conn, int argc OVS_UNUSED,
> > char *response;
> > dpdk_port_t port_id;
> > struct netdev_dpdk *dev;
> > - struct rte_eth_dev_info dev_info;
> > + struct rte_device *rte_dev;
> > + struct rte_dev_iterator iterator;
> >
> > ovs_mutex_lock(&dpdk_mutex);
> >
> > - if (rte_eth_dev_get_port_by_name(argv[1], &port_id)) {
> > + RTE_ETH_FOREACH_MATCHING_DEV(port_id, argv[1], &iterator) {
> > + break;
> > + }
> > + if (port_id == DPDK_ETH_PORT_ID_INVALID) {
> > response = xasprintf("Device '%s' not found in DPDK", argv[1]);
> > goto error;
> > }
> > @@ -3251,15 +3298,22 @@ netdev_dpdk_detach(struct unixctl_conn
> *conn, int argc OVS_UNUSED,
> > goto error;
> > }
> >
> > - rte_eth_dev_close(port_id);
> > + rte_dev = rte_eth_devices[port_id].device;
> > + if (netdev_dpdk_get_num_ports(rte_dev)) {
> > + response = xasprintf("Device '%s' is being shared with other "
> > + "interfaces. Remove them before detaching.",
> > + argv[1]);
> > + goto error;
> > + }
> >
> > - rte_eth_dev_info_get(port_id, &dev_info);
> > - if (!dev_info.device || rte_dev_remove(dev_info.device)) {
> > - response = xasprintf("Device '%s' can not be detached", argv[1]);
> > + rte_eth_dev_close(port_id);
> > + if (rte_dev_remove(rte_dev) < 0) {
> > + response = xasprintf("Device '%s' can not be removed",
> > + argv[1]);
> > goto error;
> > }
> >
> > - response = xasprintf("Device '%s' has been detached", argv[1]);
> > + response = xasprintf("All devices shared with device '%s' "
> > + "have been detached", argv[1]);
> >
> > ovs_mutex_unlock(&dpdk_mutex);
> > unixctl_command_reply(conn, response);
More information about the dev
mailing list