On 17.01.2019 3:25, Ophir Munk wrote:
> Hi Ilya,
> Thanks for the quick review.
> Please find comments inline.
>
>> -----Original Message-----
>> From: Ilya Maximets <i.maxim...@samsung.com>
>> Sent: Wednesday, January 16, 2019 5:42 PM
>> To: Ophir Munk <ophi...@mellanox.com>; ovs-dev@openvswitch.org; Ian
>> Stokes <ian.sto...@intel.com>
>> Cc: Asaf Penso <as...@mellanox.com>; Shahaf Shuler
>> <shah...@mellanox.com>; Thomas Monjalon <tho...@monjalon.net>; Olga
>> Shern <ol...@mellanox.com>; Kevin Traynor <ktray...@redhat.com>; Aaron
>> Conole <acon...@redhat.com>
>> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
>>
>> Not a full review.
>> Comments inline.
>>
>> Best regards, Ilya Maximets.
>>
>>>
>>> @@ -1361,12 +1382,32 @@ netdev_dpdk_destruct(struct netdev *netdev)
>>> dev->started = false;
>>>
>>> if (dev->attached) {
>>> + /* Retrieve eth device data before closing it.
>>> + * FIXME: avoid direct access to DPDK internal array
>>> rte_eth_devices.
>>> + */
>>> + eth_dev = &rte_eth_devices[dev->port_id];
>>> + uint32_t remove_on_close =
>>> + eth_dev->data->dev_flags & RTE_ETH_DEV_CLOSE_REMOVE;
>>
>> All other variables declared at the start of this function.
>> Please, be consistent.
>>
>
> In v6
>
>>> + rte_dev = eth_dev->device;
>>> +
>>> + /* Remove the 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);
>>> +
>>> + /* Remove this rte device and all its eth devices if flag
>>> + * RTE_ETH_DEV_CLOSE_REMOVE is not supported (which means
>> representors
>>> + * are not supported), or if all the eth devices belonging to the
>>> rte
>>> + * device are closed.
>>> + */
>>> + if (!remove_on_close || !netdev_dpdk_get_num_ports(rte_dev)) {
>>> + if (rte_dev_remove(rte_dev) < 0) {
>>> + VLOG_ERR("Device '%s' can not be detached", dev->devargs);
>>> + } else {
>>> + /* Device was closed and detached. */
>>> + VLOG_INFO("Device '%s' has been detached", dev->devargs);
>>> + }
>>> } else {
>>> - VLOG_ERR("Device '%s' can not be detached", dev->devargs);
>>> + /* Device was only closed. rte_dev_remove() was not called. */
>>> + VLOG_INFO("Device '%s' has been removed", dev->devargs);
>>
>> IMHO, this should be printed regardless of detaching. (Before it)
>>
>
> As I suggested to Kevin in a recent review - "Device has been detached" was
> changed to "Device has been removed and detached".
> This will accurately reflect the operations on the device.
>
>>> +
>>> +static int
>>> +netdev_dpdk_check_dup_dev(dpdk_port_t port_id, struct netdev *netdev,
>>> + const char *devargs, char **errp) {
>>
>> OVS_REQUIRES(dpdk_mutex)
>>
>> Also, the '{' should be on a new line in function definitions.
>>
>
> This function is removed in v6
>
>>> + struct netdev_dpdk *dup_dev;
>>> +
>>> + dup_dev = netdev_dpdk_lookup_by_port_id(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), devargs,
>>> + netdev_get_name(&dup_dev->up));
>>> + return EADDRINUSE;
>>> + }
>>> + return 0;
>>
>> Indents are off in the previous 3 lines.
>>
>
> In v6
>
>>> +}
>>> +
>>> /*
>>> - * 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.
>>> *
>>> @@ -1641,32 +1728,35 @@ netdev_dpdk_get_port_by_mac(const char
>> *mac_str)
>>> *
>>> * Note that the compatibility is fully kept: user can still use the
>>> * PCI id for adding ports (when it's enough for them).
>>> + *
>>> + * In order to avoid clang errors add OVS_REQUIRES(dpdk_mutex) to
>>> + this function
>>> + * since it is required by netdev_dpdk_lookup_by_port_id() while the
>>> + dpdk_mutex
>>> + * is taken outside of this function.
>>
>> Agree with Aaron that the comment is not needed.
>>
>
> Comment removed in v6
>
>>> */
>>> static dpdk_port_t
>>> netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
>>> const char *devargs, char **errp)
>>> + OVS_REQUIRES(dpdk_mutex)
>>> {
>>> - 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);
>>> + new_port_id = netdev_dpdk_probe_port_by_devargs(devargs);
>>> + if (!rte_eth_dev_is_valid_port(new_port_id)) {
>>> + new_port_id = DPDK_ETH_PORT_ID_INVALID;
>>> + } else {
>>> + if (netdev_dpdk_check_dup_dev(new_port_id, &dev->up,
>>> + devargs, errp)) {
>>> + new_port_id = DPDK_ETH_PORT_ID_INVALID;
>>
>> Still don't understand why you need to check duplicated ports twice.
>> Upper layer should handle this case.
>> Also, indents are off.
>
> Duplicate check is removed in v6
>
>>> +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);
>>
>> Indents off.
>> It's better to be:
>>
>> err = netdev_dpdk_check_dup_dev(new_port_id, netdev,
>> new_devargs, errp);
>>
>
> In v6 this function call is replaced by the function contents.
>
>>> @@ -3253,15 +3342,26 @@ netdev_dpdk_detach(struct unixctl_conn *conn,
>> int argc OVS_UNUSED,
>>> goto error;
>>> }
>>>
>>> - rte_eth_dev_close(port_id);
>>> + /* FIXME: avoid direct access to DPDK internal array rte_eth_devices.
>>> */
>>> + eth_dev = &rte_eth_devices[port_id];
>>> + rte_dev = eth_dev->device;
>>> +
>>> + if ((eth_dev->data->dev_flags & RTE_ETH_DEV_CLOSE_REMOVE) &&
>>> + (netdev_dpdk_get_num_ports(rte_dev) > 1)) {
>
> This line requires a fix. It should be ">=1" instead of ">1".
But current device is not closed yet.
It'll be counted by netdev_dpdk_get_num_ports().
> The two lines above are rewritten in v6.
>
>>
>> We still need to close the port here because if you'll have 2 sibling ports
>> you'll
>> not be able do detach any of them because another one still open.
>>
>> Anyway, Logic seems wrong here.
>>
>
> I will try to explain the logic
>
>> The function intended to detach pci devices. And the usecase is detaching
>> devices that was added by the whitelist and not attached in
>> 'netdev_dpdk_process_devargs', i.e. has 'dev->attached = false'.
>>
>
> Correct
>
>> The workflow should be:
>>
>> * Iterate over all the siblings of the desired device.
>> * if port_id still used by OVS (netdev_dpdk_lookup_by_port_id(port_id)
>> !=
>> NULL)
>> print "Device '%s' is being used by interface '%s'. Remove it
>> before
>> detaching"
>>
>
> For PMDs not supporting representors the code is backward compatible with
> previous versions.
> For PMDs supporting representors and in cases we have dozens of multi-ports
> (representors/interfaces) - is it desirable to print dozens of such messages?
> The current patch suggests one message for all cases:
> "Device '%s' is being shared with other interfaces. Remove them before
> detaching.",
> IMHO, I would leave it this way for this patch. If you still think it is
> useful to have a message per interface - I suggest adding it in a follow up
> patch.
> What do you think?
This function used only in manual appctl command. It's not an issue to
have a big output. By the way, you may collect all the port names and print
like this:
struct netdev_dpdk *dev;
struct ds used_interfaces = DS_EMPTY_INITIALIZER;
bool used = false;
ds_put_format(&used_interfaces,
"Device '%s' is being used by following interfaces:", argv[1]);
FOR_EACH_SIBLING (&port_id) {
dev = netdev_dpdk_lookup_by_port_id(port_id);
if (dev) {
used = true;
ds_put_format(&used_interfaces, " %s", netdev_get_name(&dev->up));
}
}
if (used) {
ds_put_cstr(&used_interfaces, ". Remove them before detaching.");
response = ds_steal_cstr(&used_interfaces);
ds_destroy(&used_interfaces);
goto error;
}
ds_destroy(&used_interfaces);
>
>> * If some ports are still used by OVS
>> return
>>
>> * If none of the ports still used by OVS
>> Iterate over all the siblings and close them.
>> (rte_eth_dev_close(port_id))
>> Try to remove the device. (rte_dev_remove(rte_dev))
>
> The responsibility for closing all eth devices before detaching the rte
> device should be embedded in the rte_dev_remove() call itself.
> In other words, when calling rte_dev_remove() - all opened eth devices should
> be automatically closed and then the rte device should be unplugged/detached.
> It requires a PMD supporting representors to implement the remove() callback
> as part of struct rte_pci_driver.
> Hence, if none of the sibling ports is used by OVS - it is sufficient to call
> rte_dev_remove(rte_dev) and there is no need to iterate over all the siblings
> and close them.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
