[ovs-dev] [PATCH] netdev-dpdk: fix port addition for ports sharing same PCI id
Yuanhan Liu
yliu at fridaylinux.org
Tue Jan 9 04:13:48 UTC 2018
On Mon, Jan 08, 2018 at 01:10:10PM +0000, Stokes, Ian wrote:
> Hi Yuanhan,
>
> Thanks for working on this, I've done some testing with ani40e device and a review of the current patch, please find comments below.
>
> > On 12/20/2017 04:03 PM, Yuanhan Liu wrote:
> > > Some NICs have only one PCI address associated with multiple ports.
> > > This patch extends the dpdk-devargs option's format to cater for such
> > devices.
> > >
> > > To achieve that, this patch uses a new syntax that will be adapted and
> > > implemented in future DPDK release (likely, v18.05):
> > > http://dpdk.org/ml/archives/dev/2017-December/084234.html
> > >
> > > And since it's the DPDK duty to parse the (complete and full) syntax
> > > and this patch is more likely to serve as an intermediate workaround,
> > > here I take a simpler and shorter syntax from it (note it's allowed to
> > > have only one category being provided):
> > > class=eth,mac=00:11:22:33:44:55:66
> > >
> > > Also, old compatibility is kept. Users can still go on with using the
> > > PCI id to add a port (if that's enough for them). Meaning, this patch
> > > will not break anything.
>
> I've validated that both the old method/new method both work with i40e devices as expected.
Thanks a lot for testing!
> > Hi Yuanhan, I think there would need to be some doc updates also for a new
> > syntax.
>
> Fully agree, would need updates to the relevant OVS DPDK related docs as well as an explanation regarding why there are 2 approaches to adding ports.
Sure, I will add it in v2. Documentation/howto/dpdk.rst is the one to
update, right?
> >
> > How settled/agreed is the syntax in DPDK now? Ideally it is totally
> > settled and we use it for these types of devices.
It's not 100% settled, but I think we are really close to it.
> > But if not...then considering we will continue to keep compatibility with
> > older simpler "pci" anyway, maybe it would be safer to add something
> > simple now like "pci","mac" or just "mac" and keep compatibility for that
> > when the new syntax is finally agreed in DPDK. It may mean some parsing to
> > distinguish pci from mac, or vdev from pci,mac though.
> >
> > Anyway, agreed syntax in DPDK is better so hopefully that can be done.
> >
> > > This patch is basically based on the one from Ciara:
> > >
> > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339496.htm
> > > l
> > >
> > > Cc: Loftus Ciara <ciara.loftus at intel.com>
> > > Cc: Thomas Monjalon <thomas at monjalon.net>
> > > Cc: Kevin Traynor <ktraynor at redhat.com>
> > > Signed-off-by: Yuanhan Liu <yliu at fridaylinux.org>
> > > ---
> > > lib/netdev-dpdk.c | 77
> > > ++++++++++++++++++++++++++++++++++++++++++++-----------
> > > 1 file changed, 62 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > > 45fcc74..4e5cc25 100644
> > > --- a/lib/netdev-dpdk.c
> > > +++ b/lib/netdev-dpdk.c
> > > @@ -1205,30 +1205,77 @@ netdev_dpdk_lookup_by_port_id(dpdk_port_t
> > port_id)
> > > return NULL;
> > > }
> > >
> > > +static int
> > > +netdev_dpdk_str_to_ether(const char *mac, struct ether_addr *ea) {
> > > + unsigned int bytes[6];
> > > + int i;
> > > +
> > > + if (sscanf(mac, "%x:%x:%x:%x:%x:%x",
> > > + &bytes[0], &bytes[1], &bytes[2],
> > > + &bytes[3], &bytes[4], &bytes[5]) != 6) {
> > > + return -1;
>
> Should an error be logged here?
I thought an error (which will be threw out when port finding is failed) is
enough? Just like we didn't do the sanity check to pci id.
Anyway, I have no objection to add an error log to make it more explicit.
> I flag the same question when checking the return type for this function later but something to think about. The error log could be here or after the return type check but I do think it's useful.
I will put the error log here (inside the if clause).
> > > + }
> > > +
> > > + for (i = 0; i < 6; i++) {
> > > + ea->addr_bytes[i] = bytes[i];
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static dpdk_port_t
> > > +netdev_dpdk_get_port_by_mac(const char *mac_str) {
> > > + int i;
> > > + struct ether_addr mac;
> > > + struct ether_addr port_mac;
> > > +
> > > + netdev_dpdk_str_to_ether(mac_str, &mac);
>
> I think there should be an error check for the call to netdev_dpdk_str_to_ether() above and an associated error log, in the case where sscanf fails within netdev_dpdk_str_to_ether() (i.e. Mac is too long, too short etc.) it will help with the logs to zero in on the issue.
Right, I missed the sanity check here.
> > > + for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> > > + if (!rte_eth_dev_is_valid_port(i)) {
> > > + continue;
> > > + }
> > > +
> > > + rte_eth_macaddr_get(i, &port_mac);
> > > + if (is_same_ether_addr(&mac, &port_mac)) {
> > > + return i;
> > > + }
> > > + }
> > > +
> > > + return DPDK_ETH_PORT_ID_INVALID;
> > > +}
> > > +
>
> In general for this function I'd like to see a comment added explaining the behavior now that there are 2 methods to add ports.
Sure, will do that in v2.
> > > static dpdk_port_t
> > > netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
> > > const char *devargs, char **errp) {
> > > - /* Get the name up to the first comma. */
> > > - char *name = xmemdup0(devargs, strcspn(devargs, ","));
> > > + char *name;
> > > dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
> > >
> > > - 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_eth_dev_attach(devargs, &new_port_id)) {
> > > - /* Attach successful */
> > > - dev->attached = true;
> > > - VLOG_INFO("Device '%s' attached to DPDK", devargs);
> > > - } else {
> > > - /* Attach unsuccessful */
> > > - new_port_id = DPDK_ETH_PORT_ID_INVALID;
> > > - VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
> > > - devargs);
> > > + if (strncmp(devargs, "class=eth,mac=", 14) == 0) {
> > > + new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]);
> > > + } else {
> >
> > It means that hotplug attach will not work for these devices - that needs
> > to be documented too. Ian also mentioned concerns about how detach would
> > work in one of the previous threads
> > https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339738.html
>
> +1 on hotplug functionality, needs to be documented when it is available and how different port attach methods will interact with it.
Unfortunately, it could not support hotplug functionality at this stage.
I think it will be resolved (naturally) when we have adapt the new syntax
in DPDK (which is targeted to v18.05 release). I will also mention this
in the doc.
Thanks for the review!
--yliu
More information about the dev
mailing list