Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

Hi Ian,
Please see inline.

> -Original Message-
> From: Stokes, Ian 
> Sent: Thursday, January 17, 2019 6:15 PM
> To: Stokes, Ian ; Ophir Munk
> ; Kevin Traynor ; ovs-
> d...@openvswitch.org
> Cc: Ilya Maximets 
> Subject: RE: [PATCH v5] netdev-dpdk: support port representors
> 
> > > Hi Ian,
> > >
> > > > -Original Message-
> > > > From: Stokes, Ian 
> > > > Sent: Wednesday, January 16, 2019 12:37 AM
> > > > To: Kevin Traynor ; Ophir Munk
> > > > ; ovs-dev@openvswitch.org
> > > > Cc: Olga Shern ; Ilya Maximets
> > > > 
> > > > Subject: RE: [PATCH v5] netdev-dpdk: support port representors
> > > >
> > > > > On 01/15/2019 09:47 AM, Ophir Munk wrote:
> > > > > > 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 :08:00.0:
> > > > > > ":08:00.0,representor=[3]" and ":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, I had a scan through and there isn't any
> > > > > documentation/examples for this outside the commit message. I
> > > > > think a user would need something basic, or at least reference
> > > > > to know that this
> > > > exists and how to use it.
> > > >
> > > > +1, although I can confirm the backwards compatibility with the
> > > > +legacy pci
> > > > port methodology I'm seeing issues around the use of representors
> > > > which I'm not entirely sure are user configuration related or
> > > > specific to the patch implementation, will need more time to
> > > > investigate and
> > > confirm.
> > > >
> > > > Ian
> > >
> > > Can you please inform what exact issues you are seeing with
> > representors?
> >
> > Hi Ophir, in my case trying to add an i40e port representor was
> > failing with invalid port.
> >
> > I want to confirm this is not an issue in DPDK, or a configuration
> > issue on my side, or that it's just not supported (although I would
> > have thought it was).
> >
> > > What is the configuration?
> > PF I40e address with 05:00.0 is bound to igb_uio.
> > VF then created for PF with 'echo 1 >
> > /sys/bus/pci/devices/\:05\:00.0/max_vfs'.
> > VF is created with address 05:02.0
> > VF then bound to igb_uio driver.
> > OVS DPDK then stated (no new commands are used here when launching).
> > Add bridge br0
> > Attempt to add VF with port representor, use the PF address and
> > representor of VF
> >
> > ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
> > options:dpdk-devargs=:05:00.0,representor=[0]
> >
> > Possibly it's above where I'm going wrong. Either on the address
> > passed, or the representor value used (though I've tried a range of
> > represntor values besides 0, no luck, how do you typically identify
> > the value?)
> >
> > > What is the NIC under test?
> > The NIC is Intel X710 (Ethernet Controller X710 for 10GbE SFP+ 1572).
> > The VF created is recognized as Ethernet Virtual Function 700 Series
> > 154c
> >
> > > What is the expected result?
> > I was attempting to see if the port representor could be used to add a
> > VF for the i40e PF.
> > There is no issue if I add the VF using the legacy method ' ovs-vsctl
> > add- port br0 dpdk0 -- set Interface dpdk0 type=dpdk options:dpdk-
> > devargs=:05:02.0'.
> >
> > > What is the actual result?
> >
> > 2019-01-16T10:18:38Z|00062|dpdk|INFO|EAL: PCI device :05:00.0 on
> > NUMA socket 0
> > 2019-01-16T10:18:38Z|00063|dpdk|ERR|EAL: Failed to attach device on
> > primary process 2019-01-16T10:18:38Z|00064|netdev_dpdk|WARN|Error
> > attaching device ':05:00.0,representor=[0]' to DPDK
> > 2019-01-16T10:18:38Z|00065|netdev|WARN|dpdk0: could not set
> > configuration (Invalid argument)
> > 2019-01-16T10:18:38Z|00066|dpdk|ERR|Invalid port_id=32
> > >
> > > Without those details I do not know how the "issues"  can be addressed.
> >
> > As I said this could be a user configuration issue, I spoke with Awal
> > co- authored the i40e port representor code which is how I arrived at
> > the 

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

> -Original Message-
> From: Ilya Maximets 
> Sent: Thursday, January 17, 2019 6:19 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
> Stokes 
> Cc: Asaf Penso ; Shahaf Shuler
> ; Thomas Monjalon ; Olga
> Shern ; Kevin Traynor ; Aaron
> Conole 
> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
> 
> On 17.01.2019 19:03, Ophir Munk wrote:
> > Hi Ilya,
> > Please see inline
> >
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Thursday, January 17, 2019 5:34 PM
> >> To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
> >> Stokes 
> >> Cc: Asaf Penso ; Shahaf Shuler
> >> ; Thomas Monjalon ;
> Olga
> >> Shern ; Kevin Traynor ;
> >> Aaron Conole 
> >> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
> >>
> >> On 17.01.2019 18:12, Ophir Munk wrote:
> >>> Hi Ilay,
> >>> Please see inline
> >>>
>  -Original Message-
>  From: Ilya Maximets 
>  Sent: Thursday, January 17, 2019 12:50 PM
>  To: Ophir Munk ; ovs-dev@openvswitch.org;
> Ian
>  Stokes 
>  Cc: Asaf Penso ; Shahaf Shuler
>  ; Thomas Monjalon ;
> >> Olga
>  Shern ; Kevin Traynor ;
>  Aaron Conole 
>  Subject: Re: [PATCH v5] netdev-dpdk: support port representors
> 
>  On 17.01.2019 3:25, Ophir Munk wrote:
> >>> +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().
> 
> >>>
> >>> This line will be eventually deleted in v6 based on your code
> >>> suggestion below
> >>>
> > 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(_interfaces,
> "Device '%s' is being used by following
>  interfaces:", argv[1]);
> 
>   FOR_EACH_SIBLING (_id) {
>   dev = netdev_dpdk_lookup_by_port_id(port_id);
>   if (dev) {
>   used = true;
>   ds_put_format(_interfaces, " %s",
>  netdev_get_name(
> >>> up));
>   }
>   }
> 
>   if (used) {
>   ds_put_cstr(_interfaces, ". Remove them before detaching.");
>   response = ds_steal_cstr(_interfaces);
>   ds_destroy(_interfaces);
>   goto error;
>   }
> 
>   ds_destroy(_interfaces);
> 
> >>>
> >>> Many thanks for the nice code suggestion!. I used most of it
> >>> (however
> >> iterating over the netdev_dpdk devices rather than over the DPDK ports).
> >>> If you agree - please become co-author of this patch.
> >>>
> >>
> >> If you wish.
> >>
> >
> > Yes, added you as co-author (need to sign-off?)
> 
> Yes. Sign-off required. Checkpatch will complain.
> 
> You could run ./utilities/checkpatch.sh  before sending.
> Or even add a commit-message hook for that.
> 

I thought sign-off must be added only by you.
I assume no need for v7 just for adding signed-off-by

> >
> >> Looking forward for v6 for the next round of review.
> >>

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ilya Maximets]

On 17.01.2019 19:03, Ophir Munk wrote:
> Hi Ilya,
> Please see inline
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Thursday, January 17, 2019 5:34 PM
>> To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
>> Stokes 
>> Cc: Asaf Penso ; Shahaf Shuler
>> ; Thomas Monjalon ; Olga
>> Shern ; Kevin Traynor ; Aaron
>> Conole 
>> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
>>
>> On 17.01.2019 18:12, Ophir Munk wrote:
>>> Hi Ilay,
>>> Please see inline
>>>
 -Original Message-
 From: Ilya Maximets 
 Sent: Thursday, January 17, 2019 12:50 PM
 To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
 Stokes 
 Cc: Asaf Penso ; Shahaf Shuler
 ; Thomas Monjalon ;
>> Olga
 Shern ; Kevin Traynor ;
 Aaron Conole 
 Subject: Re: [PATCH v5] netdev-dpdk: support port representors

 On 17.01.2019 3:25, Ophir Munk wrote:
>>> +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().

>>>
>>> This line will be eventually deleted in v6 based on your code
>>> suggestion below
>>>
> 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(_interfaces,
"Device '%s' is being used by following interfaces:",
 argv[1]);

  FOR_EACH_SIBLING (_id) {
  dev = netdev_dpdk_lookup_by_port_id(port_id);
  if (dev) {
  used = true;
  ds_put_format(_interfaces, " %s", netdev_get_name(
>>> up));
  }
  }

  if (used) {
  ds_put_cstr(_interfaces, ". Remove them before detaching.");
  response = ds_steal_cstr(_interfaces);
  ds_destroy(_interfaces);
  goto error;
  }

  ds_destroy(_interfaces);

>>>
>>> Many thanks for the nice code suggestion!. I used most of it (however
>> iterating over the netdev_dpdk devices rather than over the DPDK ports).
>>> If you agree - please become co-author of this patch.
>>>
>>
>> If you wish.
>>
> 
> Yes, added you as co-author (need to sign-off?)

Yes. Sign-off required. Checkpatch will complain.

You could run ./utilities/checkpatch.sh  before sending.
Or even add a commit-message hook for that.

> 
>> Looking forward for v6 for the next round of review.
>>
> 
> V6 was sent (my mail server may delay the sending for unknown reasons).
> 
> 
>> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Stokes, Ian]

> > Hi Ian,
> >
> > > -Original Message-
> > > From: Stokes, Ian 
> > > Sent: Wednesday, January 16, 2019 12:37 AM
> > > To: Kevin Traynor ; Ophir Munk
> > > ; ovs-dev@openvswitch.org
> > > Cc: Olga Shern ; Ilya Maximets
> > > 
> > > Subject: RE: [PATCH v5] netdev-dpdk: support port representors
> > >
> > > > On 01/15/2019 09:47 AM, Ophir Munk wrote:
> > > > > 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 :08:00.0:
> > > > > ":08:00.0,representor=[3]" and ":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, I had a scan through and there isn't any
> > > > documentation/examples for this outside the commit message. I
> > > > think a user would need something basic, or at least reference to
> > > > know that this
> > > exists and how to use it.
> > >
> > > +1, although I can confirm the backwards compatibility with the
> > > +legacy pci
> > > port methodology I'm seeing issues around the use of representors
> > > which I'm not entirely sure are user configuration related or
> > > specific to the patch implementation, will need more time to
> > > investigate and
> > confirm.
> > >
> > > Ian
> >
> > Can you please inform what exact issues you are seeing with
> representors?
> 
> Hi Ophir, in my case trying to add an i40e port representor was failing
> with invalid port.
> 
> I want to confirm this is not an issue in DPDK, or a configuration issue
> on my side, or that it's just not supported (although I would have thought
> it was).
> 
> > What is the configuration?
> PF I40e address with 05:00.0 is bound to igb_uio.
> VF then created for PF with 'echo 1 >
> /sys/bus/pci/devices/\:05\:00.0/max_vfs'.
> VF is created with address 05:02.0
> VF then bound to igb_uio driver.
> OVS DPDK then stated (no new commands are used here when launching).
> Add bridge br0
> Attempt to add VF with port representor, use the PF address and
> representor of VF
> 
> ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk
> options:dpdk-devargs=:05:00.0,representor=[0]
> 
> Possibly it's above where I'm going wrong. Either on the address passed,
> or the representor value used (though I've tried a range of represntor
> values besides 0, no luck, how do you typically identify the value?)
> 
> > What is the NIC under test?
> The NIC is Intel X710 (Ethernet Controller X710 for 10GbE SFP+ 1572).
> The VF created is recognized as Ethernet Virtual Function 700 Series 154c
> 
> > What is the expected result?
> I was attempting to see if the port representor could be used to add a VF
> for the i40e PF.
> There is no issue if I add the VF using the legacy method ' ovs-vsctl add-
> port br0 dpdk0 -- set Interface dpdk0 type=dpdk options:dpdk-
> devargs=:05:02.0'.
> 
> > What is the actual result?
> 
> 2019-01-16T10:18:38Z|00062|dpdk|INFO|EAL: PCI device :05:00.0 on NUMA
> socket 0
> 2019-01-16T10:18:38Z|00063|dpdk|ERR|EAL: Failed to attach device on
> primary process 2019-01-16T10:18:38Z|00064|netdev_dpdk|WARN|Error
> attaching device ':05:00.0,representor=[0]' to DPDK
> 2019-01-16T10:18:38Z|00065|netdev|WARN|dpdk0: could not set configuration
> (Invalid argument) 2019-01-16T10:18:38Z|00066|dpdk|ERR|Invalid port_id=32
> >
> > Without those details I do not know how the "issues"  can be addressed.
> 
> As I said this could be a user configuration issue, I spoke with Awal co-
> authored the i40e port representor code which is how I arrived at the
> commands above but these could be wrong.

Hi Ophir,

Quick update on this. I've managed to get the port representors working to an 
extent for i40e and ixgbe devices with this patch but there are a few issues 
causing failures.

ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk 
options:dpdk-devargs=:05:00.0,representor=[0]

will succeed if both the PF and the VF are hotplugged to the IGB_UIO driver 
after OVS has launched.

However it will fail if PF has already been bound to 

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

Hi Ilya,
Please see inline

> -Original Message-
> From: Ilya Maximets 
> Sent: Thursday, January 17, 2019 5:34 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
> Stokes 
> Cc: Asaf Penso ; Shahaf Shuler
> ; Thomas Monjalon ; Olga
> Shern ; Kevin Traynor ; Aaron
> Conole 
> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
> 
> On 17.01.2019 18:12, Ophir Munk wrote:
> > Hi Ilay,
> > Please see inline
> >
> >> -Original Message-
> >> From: Ilya Maximets 
> >> Sent: Thursday, January 17, 2019 12:50 PM
> >> To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
> >> Stokes 
> >> Cc: Asaf Penso ; Shahaf Shuler
> >> ; Thomas Monjalon ;
> Olga
> >> Shern ; Kevin Traynor ;
> >> Aaron Conole 
> >> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
> >>
> >> On 17.01.2019 3:25, Ophir Munk wrote:
> > +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().
> >>
> >
> > This line will be eventually deleted in v6 based on your code
> > suggestion below
> >
> >>> 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(_interfaces,
> >>"Device '%s' is being used by following interfaces:",
> >> argv[1]);
> >>
> >>  FOR_EACH_SIBLING (_id) {
> >>  dev = netdev_dpdk_lookup_by_port_id(port_id);
> >>  if (dev) {
> >>  used = true;
> >>  ds_put_format(_interfaces, " %s", netdev_get_name(
> >up));
> >>  }
> >>  }
> >>
> >>  if (used) {
> >>  ds_put_cstr(_interfaces, ". Remove them before detaching.");
> >>  response = ds_steal_cstr(_interfaces);
> >>  ds_destroy(_interfaces);
> >>  goto error;
> >>  }
> >>
> >>  ds_destroy(_interfaces);
> >>
> >
> > Many thanks for the nice code suggestion!. I used most of it (however
> iterating over the netdev_dpdk devices rather than over the DPDK ports).
> > If you agree - please become co-author of this patch.
> >
> 
> If you wish.
> 

Yes, added you as co-author (need to sign-off?)

> Looking forward for v6 for the next round of review.
> 

V6 was sent (my mail server may delay the sending for unknown reasons).


> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ilya Maximets]

On 17.01.2019 18:12, Ophir Munk wrote:
> Hi Ilay,
> Please see inline
> 
>> -Original Message-
>> From: Ilya Maximets 
>> Sent: Thursday, January 17, 2019 12:50 PM
>> To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
>> Stokes 
>> Cc: Asaf Penso ; Shahaf Shuler
>> ; Thomas Monjalon ; Olga
>> Shern ; Kevin Traynor ; Aaron
>> Conole 
>> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
>>
>> On 17.01.2019 3:25, Ophir Munk wrote:
> +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().
>>
> 
> This line will be eventually deleted in v6 based on your code suggestion below
> 
>>> 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(_interfaces,
>>"Device '%s' is being used by following interfaces:", 
>> argv[1]);
>>
>>  FOR_EACH_SIBLING (_id) {
>>  dev = netdev_dpdk_lookup_by_port_id(port_id);
>>  if (dev) {
>>  used = true;
>>  ds_put_format(_interfaces, " %s", netdev_get_name(>up));
>>  }
>>  }
>>
>>  if (used) {
>>  ds_put_cstr(_interfaces, ". Remove them before detaching.");
>>  response = ds_steal_cstr(_interfaces);
>>  ds_destroy(_interfaces);
>>  goto error;
>>  }
>>
>>  ds_destroy(_interfaces);
>>
> 
> Many thanks for the nice code suggestion!. I used most of it (however 
> iterating over the netdev_dpdk devices rather than over the DPDK ports).
> If you agree - please become co-author of this patch.
> 

If you wish.

Looking forward for v6 for the next round of review.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

Hi Ilay,
Please see inline

> -Original Message-
> From: Ilya Maximets 
> Sent: Thursday, January 17, 2019 12:50 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
> Stokes 
> Cc: Asaf Penso ; Shahaf Shuler
> ; Thomas Monjalon ; Olga
> Shern ; Kevin Traynor ; Aaron
> Conole 
> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
> 
> On 17.01.2019 3:25, Ophir Munk wrote:
> >>> +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().
> 

This line will be eventually deleted in v6 based on your code suggestion below

> > 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(_interfaces,
>"Device '%s' is being used by following interfaces:", argv[1]);
> 
>  FOR_EACH_SIBLING (_id) {
>  dev = netdev_dpdk_lookup_by_port_id(port_id);
>  if (dev) {
>  used = true;
>  ds_put_format(_interfaces, " %s", netdev_get_name(>up));
>  }
>  }
> 
>  if (used) {
>  ds_put_cstr(_interfaces, ". Remove them before detaching.");
>  response = ds_steal_cstr(_interfaces);
>  ds_destroy(_interfaces);
>  goto error;
>  }
> 
>  ds_destroy(_interfaces);
> 

Many thanks for the nice code suggestion!. I used most of it (however iterating 
over the netdev_dpdk devices rather than over the DPDK ports).
If you agree - please become co-author of this patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ilya Maximets]

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 
>> Sent: Wednesday, January 16, 2019 5:42 PM
>> To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
>> Stokes 
>> Cc: Asaf Penso ; Shahaf Shuler
>> ; Thomas Monjalon ; Olga
>> Shern ; Kevin Traynor ; Aaron
>> Conole 
>> 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 = _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, _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(_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([14]);
>>>  } else {
>>> -name = xmemdup0(devargs, strcspn(devargs, ","));
>>> -if (rte_eth_dev_get_port_by_name(name, _port_id)
>>> -|| !rte_eth_dev_is_valid_port(new_port_id)) {
>>> -  

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

Hi Ian,
Thanks for your review.
All your comments are addressed in v6 (to be sent shortly)

Regards,
Ophir

> -Original Message-
> From: Stokes, Ian 
> Sent: Tuesday, January 15, 2019 4:23 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org
> Cc: Asaf Penso ; Shahaf Shuler
> ; Thomas Monjalon ; Olga
> Shern ; Kevin Traynor ; Ilya
> Maximets 
> 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 :08:00.0: ":08:00.0,representor=[3]" and
> > ":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(_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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

Hi Ilya,
Thanks for the quick review.
Please find comments inline.

> -Original Message-
> From: Ilya Maximets 
> Sent: Wednesday, January 16, 2019 5:42 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org; Ian
> Stokes 
> Cc: Asaf Penso ; Shahaf Shuler
> ; Thomas Monjalon ; Olga
> Shern ; Kevin Traynor ; Aaron
> Conole 
> 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 = _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, _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(_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([14]);
> >  } else {
> > -name = xmemdup0(devargs, strcspn(devargs, ","));
> > -if (rte_eth_dev_get_port_by_name(name, _port_id)
> > -|| !rte_eth_dev_is_valid_port(new_port_id)) {
> > -/* Device not found in DPDK, attempt to attach it */
> > -if 

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

> -Original Message-
> From: Kevin Traynor 
> Sent: Tuesday, January 15, 2019 4:20 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Olga Shern ; Ilya
> Maximets 
> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
> 
> On 01/15/2019 09:47 AM, Ophir Munk wrote:
> > 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 :08:00.0: ":08:00.0,representor=[3]" and
> > ":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, I had a scan through and there isn't any documentation/examples for
> this outside the commit message. I think a user would need something basic, or
> at least reference to know that this exists and how to use it.
> 
> > [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=":08:00.0,representor=[3]"}
> > Port "port-rep5"
> > Interface "port-rep5"
> > type: dpdk
> > options: {dpdk-devargs=":08:00.0,representor=[5]"}
> > ovs_version: "2.10.90"
> >
> > Signed-off-by: Ophir Munk 
> > ---
> > v1 (hwol branch):
> > 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%2Fdata=02%7C01%7Cophirmu
> %40me
> >
> llanox.com%7Cdb96f1802ee6445369db08d67af48c80%7Ca652971c7d2e4d9ba6
> a4d1
> >
> 49256f461b%7C0%7C0%7C636831588077524722sdata=U5h5dbLhDN7SX
> 5Q1gq2z
> > dybkQBOekePRDNzX26G2sLc%3Dreserved=0
> > 2. skipping count of sibling ports in case the sibling port state is
> > RTE_ETH_DEV_UNUSED
> >
> > v2 (switching to master branch):
> > 1. Update based on review comments:
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >
> chwork.ozlabs.org%2Fpatch%2F1011457%2F%232051417data=02%7C01
> %7Cop
> >
> hirmu%40mellanox.com%7Cdb96f1802ee6445369db08d67af48c80%7Ca652971
> c7d2e
> >
> 4d9ba6a4d149256f461b%7C0%7C0%7C636831588077524722sdata=EOO
> PfW8xub
> > gZUv2qmmN8WoSC4FqdnKTRe%2FUCyagRiFg%3Dreserved=0
> > 2. Send patch on master branch
> >
> > v3:
> > Add a FIXME comment regarding the direct access to DPDK
> > rte_eth_devices array
> >
> > v4:
> > 1. Add FIXME comment to every direct access to DPDK rte_eth_devices
> > array 2. Do not probe unconditionally during add-port. Instead see if
> > a device has already been probed, and if so skip the
> > rte_dev_probe(devargs)
> >
> > v5:
> > 1. Updates following v4 reviews
> > 2. Handle cases where flag RTE_ETH_DEV_CLOSE_REMOVE is not supported
> >(for PMDs not supporting representors)
> >
> >  lib/netdev-dpdk.c | 174
> > ++
> >  1 file changed, 137 insertions(+), 37 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 320422b..6099564 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -1218,6 +1218,26 @@ 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).
> > + * FIXME: avoid direct access to DPDK internal array rte_eth_devices.
> > + */
> > +static int
> > +netdev_dpdk_get_num_ports(struct rte_device *device)
> > +OVS_REQUIRES(dpdk_mutex)
> > +{
> > +struct netdev_dpdk *dev;
> > +int count = 0;
> > +
> > +LIST_FOR_EACH (dev, list_node, _list) {
> > +if (rte_eth_devices[dev->port_id].device == device
> > +&& rte_eth_devices[dev->port_id].state != RTE_ETH_DEV_UNUSED) {
> > +count++;
> > +  

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ilya Maximets]

Not a full review.
Comments inline.

Best regards, Ilya Maximets.

On 15.01.2019 12:17, Ophir Munk wrote:
> 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 :08:00.0: ":08:00.0,representor=[3]" and
> ":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].
> 
> [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=":08:00.0,representor=[3]"}
> Port "port-rep5"
> Interface "port-rep5"
> type: dpdk
> options: {dpdk-devargs=":08:00.0,representor=[5]"}
> ovs_version: "2.10.90"
> 
> Signed-off-by: Ophir Munk 
> ---
> v1 (hwol branch):
> 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://patchwork.ozlabs.org/patch/1005535/
> 2. skipping count of sibling ports in case the sibling port state is 
> RTE_ETH_DEV_UNUSED 
> 
> v2 (switching to master branch):
> 1. Update based on review comments: 
> https://patchwork.ozlabs.org/patch/1011457/#2051417
> 2. Send patch on master branch
> 
> v3:
> Add a FIXME comment regarding the direct access to DPDK rte_eth_devices array
> 
> v4:
> 1. Add FIXME comment to every direct access to DPDK rte_eth_devices array
> 2. Do not probe unconditionally during add-port. Instead see if a device has 
> already
> been probed, and if so skip the rte_dev_probe(devargs)
> 
> v5:
> 1. Updates following v4 reviews
> 2. Handle cases where flag RTE_ETH_DEV_CLOSE_REMOVE is not supported 
>(for PMDs not supporting representors)
> 
>  lib/netdev-dpdk.c | 174 
> ++
>  1 file changed, 137 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 320422b..6099564 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1218,6 +1218,26 @@ 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).
> + * FIXME: avoid direct access to DPDK internal array rte_eth_devices.
> + */
> +static int
> +netdev_dpdk_get_num_ports(struct rte_device *device)
> +OVS_REQUIRES(dpdk_mutex)
> +{
> +struct netdev_dpdk *dev;
> +int count = 0;
> +
> +LIST_FOR_EACH (dev, list_node, _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)
> @@ -1353,7 +1373,8 @@ 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;
> +struct rte_eth_dev *eth_dev;
>  
>  ovs_mutex_lock(_mutex);
>  
> @@ -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 = _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.

> +rte_dev = eth_dev->device;
> +
> +/* Remove the eth device. */
>  rte_eth_dev_close(dev->port_id);
> -rte_eth_dev_info_get(dev->port_id, _info);
> -if (dev_info.device && 

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Aaron Conole]

"Stokes, Ian"  writes:

>> 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 :08:00.0: ":08:00.0,representor=[3]" and
>> ":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.
>
>> [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=":08:00.0,representor=[3]"}
>> Port "port-rep5"
>> Interface "port-rep5"
>> type: dpdk
>> options: {dpdk-devargs=":08:00.0,representor=[5]"}
>> ovs_version: "2.10.90"
>> 
>> Signed-off-by: Ophir Munk 
>> ---
>> v1 (hwol branch):
>> 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://patchwork.ozlabs.org/patch/1005535/
>> 2. skipping count of sibling ports in case the sibling port state is
>> RTE_ETH_DEV_UNUSED
>> 
>> v2 (switching to master branch):
>> 1. Update based on review comments:
>> https://patchwork.ozlabs.org/patch/1011457/#2051417
>> 2. Send patch on master branch
>> 
>> v3:
>> Add a FIXME comment regarding the direct access to DPDK rte_eth_devices
>> array
>> 
>> v4:
>> 1. Add FIXME comment to every direct access to DPDK rte_eth_devices array
>> 2. Do not probe unconditionally during add-port. Instead see if a device
>> has already been probed, and if so skip the rte_dev_probe(devargs)
>> 
>> v5:
>> 1. Updates following v4 reviews
>> 2. Handle cases where flag RTE_ETH_DEV_CLOSE_REMOVE is not supported
>>(for PMDs not supporting representors)
>> 
>>  lib/netdev-dpdk.c | 174 ++---
>> -
>>  1 file changed, 137 insertions(+), 37 deletions(-)
>> 
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 320422b..6099564
>> 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -1218,6 +1218,26 @@ 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).
>> + * FIXME: avoid direct access to DPDK internal array rte_eth_devices.
>> + */
>> +static int
>> +netdev_dpdk_get_num_ports(struct rte_device *device)
>> +OVS_REQUIRES(dpdk_mutex)
>> +{
>> +struct netdev_dpdk *dev;
>> +int count = 0;
>> +
>> +LIST_FOR_EACH (dev, list_node, _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)
>> @@ -1353,7 +1373,8 @@ 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;
>> +struct rte_eth_dev *eth_dev;
>> 
>>  ovs_mutex_lock(_mutex);
>> 
>> @@ -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 = _eth_devices[dev->port_id];
>> +uint32_t remove_on_close =
>> +eth_dev->data->dev_flags & RTE_ETH_DEV_CLOSE_REMOVE;
>> +rte_dev = eth_dev->device;
>> +
>> +/* Remove the eth device. */
>>  rte_eth_dev_close(dev->port_id);
>> -rte_eth_dev_info_get(dev->port_id, _info);
>> -if 

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Kevin Traynor]

On 01/16/2019 11:50 AM, Thomas Monjalon wrote:
> 16/01/2019 12:38, Kevin Traynor:
>> On 01/16/2019 11:06 AM, Thomas Monjalon wrote:
>>> 16/01/2019 11:56, Kevin Traynor:
 On 01/16/2019 10:21 AM, Thomas Monjalon wrote:
> But honestly, I doubt you need to talk about representors in this patch.
> The change is to support multi-ports device generally.

 ok, then just need a way to tell OVS user that the new functionality is
 existing.
>>>
>>> Which functionality? closing a port without closing others?
>>> It looks like a bug fix to me :)
>>
>> A bug? NEVER :-)  I just mean the code is enabling some new
>> functionality for a user, so they should know that it's available now.
> 
> Yes it was a bug: if you close a Chelsio port, it was closing both ports.
> Except that, what is the functionality from your point of view?

afaics, hotplug/unplug and multi-port was not intended to be supported
in OVS previously but it was not strictly prevented either and it could
lead to what you say, so can say that is a bug, at very least in docs.

> You think we need to state that multi-ports devices are supported?
> Isn't it a statement for release notes?
> 

Yeah, I think now that it would be available and properly supported, it
would be good to add into ovs/NEWS. That is normally updated at same
time as functionality.

> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Thomas Monjalon]

16/01/2019 12:38, Kevin Traynor:
> On 01/16/2019 11:06 AM, Thomas Monjalon wrote:
> > 16/01/2019 11:56, Kevin Traynor:
> >> On 01/16/2019 10:21 AM, Thomas Monjalon wrote:
> >>> But honestly, I doubt you need to talk about representors in this patch.
> >>> The change is to support multi-ports device generally.
> >>
> >> ok, then just need a way to tell OVS user that the new functionality is
> >> existing.
> > 
> > Which functionality? closing a port without closing others?
> > It looks like a bug fix to me :)
> 
> A bug? NEVER :-)  I just mean the code is enabling some new
> functionality for a user, so they should know that it's available now.

Yes it was a bug: if you close a Chelsio port, it was closing both ports.
Except that, what is the functionality from your point of view?
You think we need to state that multi-ports devices are supported?
Isn't it a statement for release notes?


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Kevin Traynor]

On 01/16/2019 11:06 AM, Thomas Monjalon wrote:
> 16/01/2019 11:56, Kevin Traynor:
>> On 01/16/2019 10:21 AM, Thomas Monjalon wrote:
>>> 16/01/2019 11:03, Ophir Munk:
 Hi Kevin and thank you for your comments.
 Please see inline...

 From: Kevin Traynor 
> On 01/15/2019 09:47 AM, Ophir Munk wrote:
>> 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 :08:00.0: ":08:00.0,representor=[3]" and
>> ":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, I had a scan through and there isn't any documentation/examples
> for this outside the commit message. I think a user would need something
> basic, or at least reference to know that this exists and how to use it.
>>>
>>> The user needs to know the DPDK syntax to reference DPDK ports.
>>> This documentation should be provided by DPDK.
>>> We don't have a doc summarizing all at the moment (I will work on it).
>>> I suggest you collect URLs for pieces of docs and list them in OVS doc
>>> about dpdk-devargs.
>>>
>>
>> Yes, an OVS user will look at OVS docs (hopefully) but probably not
>> commit messages or C code. Also, the reference has some nice testpmd
>> examples. A simple OVS example would be nice, perhaps a section in
>> ovs/Documentation/topics/dpdk/phy.rst with an example and links?
> 
> I was thinking about these places:
>   http://docs.openvswitch.org/en/latest/howto/dpdk/#ports-and-bridges
>   http://docs.openvswitch.org/en/latest/topics/dpdk/phy/#hotplugging
> 

Yeah, that last link is the file I sent. It could be there or a section
under it. I don't mind too much where it is once it's in OVS docs.

>> [2]
>> doc/guides/prog_guide/switch_representation.rst
>>

 I now realize that the reference given in [2] requires downloading DPDK.
 In v6 I will update it with a new reference:

 https://doc.dpdk.org/guides-18.08/prog_guide/switch_representation.html
>>>
>>> No need to specify a version:
>>> https://doc.dpdk.org/guides/prog_guide/switch_representation.html
>>>
>>
>> Maybe better to specify 18.11 in case something is added/removed in
>> later DPDK.
>>
 which includes abundant of documentation/examples. 
 Can you please have a look and let me know if it is sufficient, or maybe 
 you have other ideas where more documentation should be added.
>>>
>>> There is also some documentation for representor syntax:
>>> https://doc.dpdk.org/guides/prog_guide/poll_mode_drv.html#ethernet-device-standard-device-arguments
>>>
>>> But honestly, I doubt you need to talk about representors in this patch.
>>> The change is to support multi-ports device generally.
>>
>> ok, then just need a way to tell OVS user that the new functionality is
>> existing.
> 
> Which functionality? closing a port without closing others?
> It looks like a bug fix to me :)
> 

A bug? NEVER :-)  I just mean the code is enabling some new
functionality for a user, so they should know that it's available now.

>>> The representor ports are just a use case of multi-ports.
>>> Other use cases:
>>> https://doc.dpdk.org/guides/nics/mlx4.html#implementation-details
>>> https://doc.dpdk.org/guides/nics/cxgbe.html#limitations
>>> For these use cases, we can use the mac= argument in devargs.
> 
> 
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Kevin Traynor]

On 01/16/2019 10:21 AM, Thomas Monjalon wrote:
> 16/01/2019 11:03, Ophir Munk:
>> Hi Kevin and thank you for your comments.
>> Please see inline...
>>
>> From: Kevin Traynor 
>>> On 01/15/2019 09:47 AM, Ophir Munk wrote:
 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 :08:00.0: ":08:00.0,representor=[3]" and
 ":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, I had a scan through and there isn't any documentation/examples
>>> for this outside the commit message. I think a user would need something
>>> basic, or at least reference to know that this exists and how to use it.
> 
> The user needs to know the DPDK syntax to reference DPDK ports.
> This documentation should be provided by DPDK.
> We don't have a doc summarizing all at the moment (I will work on it).
> I suggest you collect URLs for pieces of docs and list them in OVS doc
> about dpdk-devargs.
> 

Yes, an OVS user will look at OVS docs (hopefully) but probably not
commit messages or C code. Also, the reference has some nice testpmd
examples. A simple OVS example would be nice, perhaps a section in
ovs/Documentation/topics/dpdk/phy.rst with an example and links?

 [2]
 doc/guides/prog_guide/switch_representation.rst

>>
>> I now realize that the reference given in [2] requires downloading DPDK.
>> In v6 I will update it with a new reference:
>>
>> https://doc.dpdk.org/guides-18.08/prog_guide/switch_representation.html
> 
> No need to specify a version:
> https://doc.dpdk.org/guides/prog_guide/switch_representation.html
> 

Maybe better to specify 18.11 in case something is added/removed in
later DPDK.

>> which includes abundant of documentation/examples. 
>> Can you please have a look and let me know if it is sufficient, or maybe you 
>> have other ideas where more documentation should be added.
> 
> There is also some documentation for representor syntax:
> https://doc.dpdk.org/guides/prog_guide/poll_mode_drv.html#ethernet-device-standard-device-arguments
> 
> But honestly, I doubt you need to talk about representors in this patch.
> The change is to support multi-ports device generally.

ok, then just need a way to tell OVS user that the new functionality is
existing.

> The representor ports are just a use case of multi-ports.
> Other use cases:
>   https://doc.dpdk.org/guides/nics/mlx4.html#implementation-details
>   https://doc.dpdk.org/guides/nics/cxgbe.html#limitations
> For these use cases, we can use the mac= argument in devargs.
> 
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Stokes, Ian]

> Hi Ian,
> 
> > -Original Message-
> > From: Stokes, Ian 
> > Sent: Wednesday, January 16, 2019 12:37 AM
> > To: Kevin Traynor ; Ophir Munk
> > ; ovs-dev@openvswitch.org
> > Cc: Olga Shern ; Ilya Maximets
> > 
> > Subject: RE: [PATCH v5] netdev-dpdk: support port representors
> >
> > > On 01/15/2019 09:47 AM, Ophir Munk wrote:
> > > > 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 :08:00.0:
> > > > ":08:00.0,representor=[3]" and ":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, I had a scan through and there isn't any
> > > documentation/examples for this outside the commit message. I think
> > > a user would need something basic, or at least reference to know
> > > that this
> > exists and how to use it.
> >
> > +1, although I can confirm the backwards compatibility with the legacy
> > +pci
> > port methodology I'm seeing issues around the use of representors
> > which I'm not entirely sure are user configuration related or specific
> > to the patch implementation, will need more time to investigate and
> confirm.
> >
> > Ian
> 
> Can you please inform what exact issues you are seeing with representors?

Hi Ophir, in my case trying to add an i40e port representor was failing with 
invalid port.

I want to confirm this is not an issue in DPDK, or a configuration issue on my 
side, or that it's just not supported (although I would have thought it was).

> What is the configuration?
PF I40e address with 05:00.0 is bound to igb_uio.
VF then created for PF with 'echo 1 > 
/sys/bus/pci/devices/\:05\:00.0/max_vfs'.
VF is created with address 05:02.0
VF then bound to igb_uio driver.
OVS DPDK then stated (no new commands are used here when launching).
Add bridge br0
Attempt to add VF with port representor, use the PF address and representor of 
VF

ovs-vsctl add-port br0 dpdk0 -- set Interface dpdk0 type=dpdk 
options:dpdk-devargs=:05:00.0,representor=[0]

Possibly it's above where I'm going wrong. Either on the address passed, or the 
representor value used (though I've tried a range of represntor values besides 
0, no luck, how do you typically identify the value?)

> What is the NIC under test?
The NIC is Intel X710 (Ethernet Controller X710 for 10GbE SFP+ 1572).
The VF created is recognized as Ethernet Virtual Function 700 Series 154c

> What is the expected result?
I was attempting to see if the port representor could be used to add a VF for 
the i40e PF.
There is no issue if I add the VF using the legacy method ' ovs-vsctl add-port 
br0 dpdk0 -- set Interface dpdk0 type=dpdk options:dpdk-devargs=:05:02.0'. 

> What is the actual result?

2019-01-16T10:18:38Z|00062|dpdk|INFO|EAL: PCI device :05:00.0 on NUMA 
socket 0
2019-01-16T10:18:38Z|00063|dpdk|ERR|EAL: Failed to attach device on primary 
process
2019-01-16T10:18:38Z|00064|netdev_dpdk|WARN|Error attaching device 
':05:00.0,representor=[0]' to DPDK
2019-01-16T10:18:38Z|00065|netdev|WARN|dpdk0: could not set configuration 
(Invalid argument)
2019-01-16T10:18:38Z|00066|dpdk|ERR|Invalid port_id=32
> 
> Without those details I do not know how the "issues"  can be addressed.

As I said this could be a user configuration issue, I spoke with Awal 
co-authored the i40e port representor code which is how I arrived at the 
commands above but these could be wrong.

Ian
> 
> Regards,
> Ophir
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Thomas Monjalon]

16/01/2019 11:03, Ophir Munk:
> Hi Kevin and thank you for your comments.
> Please see inline...
> 
> From: Kevin Traynor 
> > On 01/15/2019 09:47 AM, Ophir Munk wrote:
> > > 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 :08:00.0: ":08:00.0,representor=[3]" and
> > > ":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, I had a scan through and there isn't any documentation/examples
> > for this outside the commit message. I think a user would need something
> > basic, or at least reference to know that this exists and how to use it.

The user needs to know the DPDK syntax to reference DPDK ports.
This documentation should be provided by DPDK.
We don't have a doc summarizing all at the moment (I will work on it).
I suggest you collect URLs for pieces of docs and list them in OVS doc
about dpdk-devargs.

> > > [2]
> > > doc/guides/prog_guide/switch_representation.rst
> > >
> 
> I now realize that the reference given in [2] requires downloading DPDK.
> In v6 I will update it with a new reference:
> 
> https://doc.dpdk.org/guides-18.08/prog_guide/switch_representation.html

No need to specify a version:
https://doc.dpdk.org/guides/prog_guide/switch_representation.html

> which includes abundant of documentation/examples. 
> Can you please have a look and let me know if it is sufficient, or maybe you 
> have other ideas where more documentation should be added.

There is also some documentation for representor syntax:
https://doc.dpdk.org/guides/prog_guide/poll_mode_drv.html#ethernet-device-standard-device-arguments

But honestly, I doubt you need to talk about representors in this patch.
The change is to support multi-ports device generally.
The representor ports are just a use case of multi-ports.
Other use cases:
https://doc.dpdk.org/guides/nics/mlx4.html#implementation-details
https://doc.dpdk.org/guides/nics/cxgbe.html#limitations
For these use cases, we can use the mac= argument in devargs.


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

Hi Ian,

> -Original Message-
> From: Stokes, Ian 
> Sent: Wednesday, January 16, 2019 12:37 AM
> To: Kevin Traynor ; Ophir Munk
> ; ovs-dev@openvswitch.org
> Cc: Olga Shern ; Ilya Maximets
> 
> Subject: RE: [PATCH v5] netdev-dpdk: support port representors
> 
> > On 01/15/2019 09:47 AM, Ophir Munk wrote:
> > > 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 :08:00.0:
> > > ":08:00.0,representor=[3]" and ":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, I had a scan through and there isn't any
> > documentation/examples for this outside the commit message. I think a
> > user would need something basic, or at least reference to know that this
> exists and how to use it.
> 
> +1, although I can confirm the backwards compatibility with the legacy pci
> port methodology I'm seeing issues around the use of representors which
> I'm not entirely sure are user configuration related or specific to the patch
> implementation, will need more time to investigate and confirm.
> 
> Ian

Can you please inform what exact issues you are seeing with representors?
What is the configuration? 
What is the NIC under test?
What is the expected result?
What is the actual result?

Without those details I do not know how the "issues"  can be addressed.

Regards,
Ophir
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

Hi Kevin and thank you for your comments.
Please see inline...

> -Original Message-
> From: Kevin Traynor 
> Sent: Tuesday, January 15, 2019 4:20 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Olga Shern ;
> Ilya Maximets 
> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
> 
> On 01/15/2019 09:47 AM, Ophir Munk wrote:
> > 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 :08:00.0: ":08:00.0,representor=[3]" and
> > ":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, I had a scan through and there isn't any documentation/examples
> for this outside the commit message. I think a user would need something
> basic, or at least reference to know that this exists and how to use it.
> 
...
> > [2]
> > doc/guides/prog_guide/switch_representation.rst
> >

I now realize that the reference given in [2] requires downloading DPDK.
In v6 I will update it with a new reference:

https://doc.dpdk.org/guides-18.08/prog_guide/switch_representation.html

which includes abundant of documentation/examples. 
Can you please have a look and let me know if it is sufficient, or maybe you 
have other ideas where more documentation should be added.

Regards,
Ophir

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Stokes, Ian]

> On 01/15/2019 09:47 AM, Ophir Munk wrote:
> > 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 :08:00.0: ":08:00.0,representor=[3]" and
> > ":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, I had a scan through and there isn't any documentation/examples
> for this outside the commit message. I think a user would need something
> basic, or at least reference to know that this exists and how to use it.

+1, although I can confirm the backwards compatibility with the legacy pci port 
methodology I'm seeing issues around the use of representors which I'm not 
entirely sure are user configuration related or specific to the patch 
implementation, will need more time to investigate and confirm.

Ian 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Kevin Traynor]

On 01/15/2019 09:47 AM, Ophir Munk wrote:
> 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 :08:00.0: ":08:00.0,representor=[3]" and
> ":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, I had a scan through and there isn't any
documentation/examples for this outside the commit message. I think a
user would need something basic, or at least reference to know that this
exists and how to use it.

> [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=":08:00.0,representor=[3]"}
> Port "port-rep5"
> Interface "port-rep5"
> type: dpdk
> options: {dpdk-devargs=":08:00.0,representor=[5]"}
> ovs_version: "2.10.90"
> 
> Signed-off-by: Ophir Munk 
> ---
> v1 (hwol branch):
> 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://patchwork.ozlabs.org/patch/1005535/
> 2. skipping count of sibling ports in case the sibling port state is 
> RTE_ETH_DEV_UNUSED 
> 
> v2 (switching to master branch):
> 1. Update based on review comments: 
> https://patchwork.ozlabs.org/patch/1011457/#2051417
> 2. Send patch on master branch
> 
> v3:
> Add a FIXME comment regarding the direct access to DPDK rte_eth_devices array
> 
> v4:
> 1. Add FIXME comment to every direct access to DPDK rte_eth_devices array
> 2. Do not probe unconditionally during add-port. Instead see if a device has 
> already
> been probed, and if so skip the rte_dev_probe(devargs)
> 
> v5:
> 1. Updates following v4 reviews
> 2. Handle cases where flag RTE_ETH_DEV_CLOSE_REMOVE is not supported 
>(for PMDs not supporting representors)
> 
>  lib/netdev-dpdk.c | 174 
> ++
>  1 file changed, 137 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 320422b..6099564 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1218,6 +1218,26 @@ 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).
> + * FIXME: avoid direct access to DPDK internal array rte_eth_devices.
> + */
> +static int
> +netdev_dpdk_get_num_ports(struct rte_device *device)
> +OVS_REQUIRES(dpdk_mutex)
> +{
> +struct netdev_dpdk *dev;
> +int count = 0;
> +
> +LIST_FOR_EACH (dev, list_node, _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)
> @@ -1353,7 +1373,8 @@ 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;
> +struct rte_eth_dev *eth_dev;
>  
>  ovs_mutex_lock(_mutex);
>  
> @@ -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 = _eth_devices[dev->port_id];
> +uint32_t remove_on_close =
> +eth_dev->data->dev_flags & RTE_ETH_DEV_CLOSE_REMOVE;
> +rte_dev = eth_dev->device;
> +
> +/* Remove the eth device. */
>  rte_eth_dev_close(dev->port_id);
> -

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Stokes, Ian]

> 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 :08:00.0: ":08:00.0,representor=[3]" and
> ":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.

> [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=":08:00.0,representor=[3]"}
> Port "port-rep5"
> Interface "port-rep5"
> type: dpdk
> options: {dpdk-devargs=":08:00.0,representor=[5]"}
> ovs_version: "2.10.90"
> 
> Signed-off-by: Ophir Munk 
> ---
> v1 (hwol branch):
> 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://patchwork.ozlabs.org/patch/1005535/
> 2. skipping count of sibling ports in case the sibling port state is
> RTE_ETH_DEV_UNUSED
> 
> v2 (switching to master branch):
> 1. Update based on review comments:
> https://patchwork.ozlabs.org/patch/1011457/#2051417
> 2. Send patch on master branch
> 
> v3:
> Add a FIXME comment regarding the direct access to DPDK rte_eth_devices
> array
> 
> v4:
> 1. Add FIXME comment to every direct access to DPDK rte_eth_devices array
> 2. Do not probe unconditionally during add-port. Instead see if a device
> has already been probed, and if so skip the rte_dev_probe(devargs)
> 
> v5:
> 1. Updates following v4 reviews
> 2. Handle cases where flag RTE_ETH_DEV_CLOSE_REMOVE is not supported
>(for PMDs not supporting representors)
> 
>  lib/netdev-dpdk.c | 174 ++---
> -
>  1 file changed, 137 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 320422b..6099564
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1218,6 +1218,26 @@ 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).
> + * FIXME: avoid direct access to DPDK internal array rte_eth_devices.
> + */
> +static int
> +netdev_dpdk_get_num_ports(struct rte_device *device)
> +OVS_REQUIRES(dpdk_mutex)
> +{
> +struct netdev_dpdk *dev;
> +int count = 0;
> +
> +LIST_FOR_EACH (dev, list_node, _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)
> @@ -1353,7 +1373,8 @@ 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;
> +struct rte_eth_dev *eth_dev;
> 
>  ovs_mutex_lock(_mutex);
> 
> @@ -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 = _eth_devices[dev->port_id];
> +uint32_t remove_on_close =
> +eth_dev->data->dev_flags & RTE_ETH_DEV_CLOSE_REMOVE;
> +rte_dev = eth_dev->device;
> +
> +/* Remove the eth device. */
>  rte_eth_dev_close(dev->port_id);
> -rte_eth_dev_info_get(dev->port_id, _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 

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ophir Munk]

My apologies. 
My outlook server has issues.
All patches are the same.

Regards,
Ophir

> -Original Message-
> From: Ilya Maximets 
> Sent: Tuesday, January 15, 2019 12:09 PM
> To: Ophir Munk ; ovs-dev@openvswitch.org
> Cc: Ian Stokes ; Olga Shern ;
> Kevin Traynor 
> Subject: Re: [PATCH v5] netdev-dpdk: support port representors
> 
> Hmm. I received 5 copies of this patch.
> Are they different or you just have some strange issues with sending mails?
> 
> Best regards, Ilya Maximets.
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] netdev-dpdk: support port representors

[Ilya Maximets]

Hmm. I received 5 copies of this patch.
Are they different or you just have some strange issues with sending mails?

Best regards, Ilya Maximets.

On 15.01.2019 12:47, Ophir Munk wrote:
> 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 :08:00.0: ":08:00.0,representor=[3]" and
> ":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].
> 
> [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=":08:00.0,representor=[3]"}
> Port "port-rep5"
> Interface "port-rep5"
> type: dpdk
> options: {dpdk-devargs=":08:00.0,representor=[5]"}
> ovs_version: "2.10.90"
> 
> Signed-off-by: Ophir Munk 
> ---
> v1 (hwol branch):
> 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://patchwork.ozlabs.org/patch/1005535/
> 2. skipping count of sibling ports in case the sibling port state is 
> RTE_ETH_DEV_UNUSED 
> 
> v2 (switching to master branch):
> 1. Update based on review comments: 
> https://patchwork.ozlabs.org/patch/1011457/#2051417
> 2. Send patch on master branch
> 
> v3:
> Add a FIXME comment regarding the direct access to DPDK rte_eth_devices array
> 
> v4:
> 1. Add FIXME comment to every direct access to DPDK rte_eth_devices array
> 2. Do not probe unconditionally during add-port. Instead see if a device has 
> already
> been probed, and if so skip the rte_dev_probe(devargs)
> 
> v5:
> 1. Updates following v4 reviews
> 2. Handle cases where flag RTE_ETH_DEV_CLOSE_REMOVE is not supported 
>(for PMDs not supporting representors)
> 
>  lib/netdev-dpdk.c | 174 
> ++
>  1 file changed, 137 insertions(+), 37 deletions(-)
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev