diff mbox

[ovs-dev,1/2] netdev-vport: Always implement get_ifindex for netdev-vport

Message ID 1502080323-11190-2-git-send-email-roid@mellanox.com
State Accepted
Headers show

Commit Message

Roi Dayan Aug. 7, 2017, 4:32 a.m. UTC 
From: Paul Blakey <paulb@mellanox.com>

Always implement get_ifindex without checking if offload is
enabled or not as this should not be related. From ovs-dpctl
we cannot tell if offload is enabled or not as other_config is
not being read.

Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
 lib/netdev-vport.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

Comments

Ben Pfaff Aug. 7, 2017, 2 p.m. UTC | #1 
On Mon, Aug 07, 2017 at 07:32:02AM +0300, Roi Dayan wrote:
> From: Paul Blakey <paulb@mellanox.com>
> 
> Always implement get_ifindex without checking if offload is
> enabled or not as this should not be related. From ovs-dpctl
> we cannot tell if offload is enabled or not as other_config is
> not being read.
> 
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>

Applied to master and branch-2.8, thanks!
Ben Pfaff Aug. 7, 2017, 5:05 p.m. UTC | #2 
On Mon, Aug 07, 2017 at 07:00:31AM -0700, Ben Pfaff wrote:
> On Mon, Aug 07, 2017 at 07:32:02AM +0300, Roi Dayan wrote:
> > From: Paul Blakey <paulb@mellanox.com>
> > 
> > Always implement get_ifindex without checking if offload is
> > enabled or not as this should not be related. From ovs-dpctl
> > we cannot tell if offload is enabled or not as other_config is
> > not being read.
> > 
> > Signed-off-by: Paul Blakey <paulb@mellanox.com>
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> 
> Applied to master and branch-2.8, thanks!

Sorry, I had to revert this because it caused several unit test
failures: 770 781 783 787 788 791 2189 2378.
Roi Dayan Aug. 8, 2017, 4:36 a.m. UTC | #3 
On 07/08/2017 20:05, Ben Pfaff wrote:
> On Mon, Aug 07, 2017 at 07:00:31AM -0700, Ben Pfaff wrote:
>> On Mon, Aug 07, 2017 at 07:32:02AM +0300, Roi Dayan wrote:
>>> From: Paul Blakey <paulb@mellanox.com>
>>>
>>> Always implement get_ifindex without checking if offload is
>>> enabled or not as this should not be related. From ovs-dpctl
>>> we cannot tell if offload is enabled or not as other_config is
>>> not being read.
>>>
>>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>>
>> Applied to master and branch-2.8, thanks!
>
> Sorry, I had to revert this because it caused several unit test
> failures: 770 781 783 787 788 791 2189 2378.
>

This is because of the warnings from get_ifindex which resolved in
the second patch but was missing the ratelimiting you mentioned.
I submitted V2 of it to add back the ratelimiting
"netdev-linux: Reduce log level for ENODEV errors getting ifindex"
Simon Horman Aug. 8, 2017, 8:04 a.m. UTC | #4 
On Tue, Aug 08, 2017 at 07:36:25AM +0300, Roi Dayan wrote:
> 
> 
> On 07/08/2017 20:05, Ben Pfaff wrote:
> >On Mon, Aug 07, 2017 at 07:00:31AM -0700, Ben Pfaff wrote:
> >>On Mon, Aug 07, 2017 at 07:32:02AM +0300, Roi Dayan wrote:
> >>>From: Paul Blakey <paulb@mellanox.com>
> >>>
> >>>Always implement get_ifindex without checking if offload is
> >>>enabled or not as this should not be related. From ovs-dpctl
> >>>we cannot tell if offload is enabled or not as other_config is
> >>>not being read.
> >>>
> >>>Signed-off-by: Paul Blakey <paulb@mellanox.com>
> >>>Reviewed-by: Roi Dayan <roid@mellanox.com>
> >>
> >>Applied to master and branch-2.8, thanks!
> >
> >Sorry, I had to revert this because it caused several unit test
> >failures: 770 781 783 787 788 791 2189 2378.
> >
> 
> This is because of the warnings from get_ifindex which resolved in
> the second patch but was missing the ratelimiting you mentioned.
> I submitted V2 of it to add back the ratelimiting
> "netdev-linux: Reduce log level for ENODEV errors getting ifindex"

In that case shouldn't the patch order be reversed to avoid
the (temporary) regression Ben pointed out?
Roi Dayan Aug. 8, 2017, 10:35 a.m. UTC | #5 
On 08/08/2017 11:04, Simon Horman wrote:
> On Tue, Aug 08, 2017 at 07:36:25AM +0300, Roi Dayan wrote:
>>
>>
>> On 07/08/2017 20:05, Ben Pfaff wrote:
>>> On Mon, Aug 07, 2017 at 07:00:31AM -0700, Ben Pfaff wrote:
>>>> On Mon, Aug 07, 2017 at 07:32:02AM +0300, Roi Dayan wrote:
>>>>> From: Paul Blakey <paulb@mellanox.com>
>>>>>
>>>>> Always implement get_ifindex without checking if offload is
>>>>> enabled or not as this should not be related. From ovs-dpctl
>>>>> we cannot tell if offload is enabled or not as other_config is
>>>>> not being read.
>>>>>
>>>>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>>>>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>>>>
>>>> Applied to master and branch-2.8, thanks!
>>>
>>> Sorry, I had to revert this because it caused several unit test
>>> failures: 770 781 783 787 788 791 2189 2378.
>>>
>>
>> This is because of the warnings from get_ifindex which resolved in
>> the second patch but was missing the ratelimiting you mentioned.
>> I submitted V2 of it to add back the ratelimiting
>> "netdev-linux: Reduce log level for ENODEV errors getting ifindex"
>
> In that case shouldn't the patch order be reversed to avoid
> the (temporary) regression Ben pointed out?
>

right.
should i post V3 for changing the order or is it something that
can be done when merged?
Simon Horman Aug. 8, 2017, 12:25 p.m. UTC | #6 
On Tue, Aug 08, 2017 at 01:35:41PM +0300, Roi Dayan wrote:
> 
> 
> On 08/08/2017 11:04, Simon Horman wrote:
> >On Tue, Aug 08, 2017 at 07:36:25AM +0300, Roi Dayan wrote:
> >>
> >>
> >>On 07/08/2017 20:05, Ben Pfaff wrote:
> >>>On Mon, Aug 07, 2017 at 07:00:31AM -0700, Ben Pfaff wrote:
> >>>>On Mon, Aug 07, 2017 at 07:32:02AM +0300, Roi Dayan wrote:
> >>>>>From: Paul Blakey <paulb@mellanox.com>
> >>>>>
> >>>>>Always implement get_ifindex without checking if offload is
> >>>>>enabled or not as this should not be related. From ovs-dpctl
> >>>>>we cannot tell if offload is enabled or not as other_config is
> >>>>>not being read.
> >>>>>
> >>>>>Signed-off-by: Paul Blakey <paulb@mellanox.com>
> >>>>>Reviewed-by: Roi Dayan <roid@mellanox.com>
> >>>>
> >>>>Applied to master and branch-2.8, thanks!
> >>>
> >>>Sorry, I had to revert this because it caused several unit test
> >>>failures: 770 781 783 787 788 791 2189 2378.
> >>>
> >>
> >>This is because of the warnings from get_ifindex which resolved in
> >>the second patch but was missing the ratelimiting you mentioned.
> >>I submitted V2 of it to add back the ratelimiting
> >>"netdev-linux: Reduce log level for ENODEV errors getting ifindex"
> >
> >In that case shouldn't the patch order be reversed to avoid
> >the (temporary) regression Ben pointed out?
> >
> 
> right.
> should i post V3 for changing the order or is it something that
> can be done when merged?

I guess its up to Ben, but surely posting v3 would not hurt.
diff mbox

Patch

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 64a3ba3..d11c5cc 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -857,7 +857,7 @@  get_pt_mode(const struct netdev *netdev)
 
 #ifdef __linux__
 static int
-netdev_vport_get_ifindex__(const struct netdev *netdev_)
+netdev_vport_get_ifindex(const struct netdev *netdev_)
 {
     char buf[NETDEV_VPORT_NAME_BUFSIZE];
     const char *name = netdev_vport_get_dpif_port(netdev_, buf, sizeof(buf));
@@ -865,15 +865,6 @@  netdev_vport_get_ifindex__(const struct netdev *netdev_)
     return linux_get_ifindex(name);
 }
 
-static int
-netdev_vport_get_ifindex(const struct netdev *netdev_)
-{
-    if (netdev_is_flow_api_enabled())
-        return netdev_vport_get_ifindex__(netdev_);
-    else
-        return -EOPNOTSUPP;
-}
-
 #define NETDEV_VPORT_GET_IFINDEX netdev_vport_get_ifindex
 #define NETDEV_FLOW_OFFLOAD_API LINUX_FLOW_OFFLOAD_API
 #else /* !__linux__ */