| Message ID | 20200507155412.3854062-3-aconole@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | TC flower support fixes | expand |
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) { >
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.
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 --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) {
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(+)