diff mbox series

[ovs-dev,2/2] netdev-offload-tc: re-fetch block ID after probing

Message ID 20200507155412.3854062-3-aconole@redhat.com
State Superseded
Delegated to: Ilya Maximets
Headers show
Series TC flower support fixes | expand

Commit Message

Aaron Conole May 7, 2020, 3:54 p.m. UTC
It's possible that port ordering could cause the block ID to change
after enabling / detecting TC Flower support.  Therefore, fetch the
block_id again after probing.

Fixes: edc2055a2bf7 ("netdev-offload-tc: Flush rules on ingress block when init tc flow api")
Cc: Dmytro Linkin <dmitrolin@mellanox.com>
Co-authored-by: Marcelo Leitner <mleitner@redhat.com>
Signed-off-by: Marcelo Leitner <mleitner@redhat.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/netdev-offload-tc.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Roi Dayan May 10, 2020, 6:23 a.m. UTC | #1
On 2020-05-07 6:54 PM, Aaron Conole wrote:
> It's possible that port ordering could cause the block ID to change
> after enabling / detecting TC Flower support.  Therefore, fetch the
> block_id again after probing.

Maybe be more specific and add that block ID could change after probe
for block support, instead of general tc flower support.

beside that looks good to me.

Acked-by: Roi Dayan <roid@mellanox.com>

> 
> Fixes: edc2055a2bf7 ("netdev-offload-tc: Flush rules on ingress block when init tc flow api")
> Cc: Dmytro Linkin <dmitrolin@mellanox.com>
> Co-authored-by: Marcelo Leitner <mleitner@redhat.com>
> Signed-off-by: Marcelo Leitner <mleitner@redhat.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/netdev-offload-tc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 875ebef719..8864555d01 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1939,6 +1939,7 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>          ovsthread_once_done(&multi_mask_once);
>      }
>  
> +    block_id = get_block_id_from_netdev(netdev);
>      error = tc_add_del_qdisc(ifindex, true, block_id, hook);
>  
>      if (error && error != EEXIST) {
>
Ilya Maximets May 15, 2020, 7:49 p.m. UTC | #2
On 5/7/20 5:54 PM, Aaron Conole wrote:
> It's possible that port ordering could cause the block ID to change
> after enabling / detecting TC Flower support.  Therefore, fetch the
> block_id again after probing.
> 
> Fixes: edc2055a2bf7 ("netdev-offload-tc: Flush rules on ingress block when init tc flow api")
> Cc: Dmytro Linkin <dmitrolin@mellanox.com>
> Co-authored-by: Marcelo Leitner <mleitner@redhat.com>
> Signed-off-by: Marcelo Leitner <mleitner@redhat.com>
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
>  lib/netdev-offload-tc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 875ebef719..8864555d01 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1939,6 +1939,7 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>          ovsthread_once_done(&multi_mask_once);
>      }
>  
> +    block_id = get_block_id_from_netdev(netdev);

This is not obvious from the code why we're fetching block id twice.
Maybe we could move this right after probing and add the comment?
Something like:

    if (ovsthread_once_start(&block_once)) {
        probe_tc_block_support(ifindex);
        /* Need to re-fetch block id since it depends on feature availability. */
        block_id = get_block_id_from_netdev(netdev);
        ovsthread_once_done(&block_once);
    }

What do you think?

Best regards, Ilya Maximets.
Marcelo Ricardo Leitner May 15, 2020, 7:58 p.m. UTC | #3
On Fri, May 15, 2020 at 09:49:15PM +0200, Ilya Maximets wrote:
> On 5/7/20 5:54 PM, Aaron Conole wrote:
> > It's possible that port ordering could cause the block ID to change
> > after enabling / detecting TC Flower support.  Therefore, fetch the
> > block_id again after probing.
> > 
> > Fixes: edc2055a2bf7 ("netdev-offload-tc: Flush rules on ingress block when init tc flow api")
> > Cc: Dmytro Linkin <dmitrolin@mellanox.com>
> > Co-authored-by: Marcelo Leitner <mleitner@redhat.com>
> > Signed-off-by: Marcelo Leitner <mleitner@redhat.com>
> > Signed-off-by: Aaron Conole <aconole@redhat.com>
> > ---
> >  lib/netdev-offload-tc.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > index 875ebef719..8864555d01 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -1939,6 +1939,7 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> >          ovsthread_once_done(&multi_mask_once);
> >      }
> >  
> > +    block_id = get_block_id_from_netdev(netdev);
> 
> This is not obvious from the code why we're fetching block id twice.
> Maybe we could move this right after probing and add the comment?
> Something like:
> 
>     if (ovsthread_once_start(&block_once)) {
>         probe_tc_block_support(ifindex);
>         /* Need to re-fetch block id since it depends on feature availability. */
>         block_id = get_block_id_from_netdev(netdev);
>         ovsthread_once_done(&block_once);
>     }
> 
> What do you think?

Yes, LGTM. Good point, it doesn't need to be re-fetched if the first
one was good already (i.e., if the block support was already checked).

Thanks,
Marcelo

> 
> Best regards, Ilya Maximets.
>
diff mbox series

Patch

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 875ebef719..8864555d01 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1939,6 +1939,7 @@  netdev_tc_init_flow_api(struct netdev *netdev)
         ovsthread_once_done(&multi_mask_once);
     }
 
+    block_id = get_block_id_from_netdev(netdev);
     error = tc_add_del_qdisc(ifindex, true, block_id, hook);
 
     if (error && error != EEXIST) {