Re: [ovs-dev] [PATCH] ovs: Only clear tstamp when changing namespaces
On Sun, Sep 19, 2021 at 10:59 AM Tyler J. Stachecki wrote: > > As of "ovs: clear skb->tstamp in forwarding path", the > tstamp is now being cleared unconditionally to fix fq qdisc > operation with ovs vports. > > While this is mostly correct and fixes forwarding for that > use case, a slight adjustment is necessary to ensure that > the tstamp is cleared *only when the forwarding is across > namespaces*. Hmm? I am sure timestamp has already been cleared when crossing netns: void skb_scrub_packet(struct sk_buff *skb, bool xnet) { ... if (!xnet) return; ipvs_reset(skb); skb->mark = 0; skb->tstamp = 0; } So, what are you trying to fix? > > Signed-off-by: Tyler J. Stachecki > --- > net/openvswitch/vport.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c > index cf2ce5812489..c2d32a5c3697 100644 > --- a/net/openvswitch/vport.c > +++ b/net/openvswitch/vport.c > @@ -507,7 +507,8 @@ void ovs_vport_send(struct vport *vport, struct sk_buff > *skb, u8 mac_proto) > } > > skb->dev = vport->dev; > - skb->tstamp = 0; > + if (dev_net(skb->dev)) Doesn't dev_net() always return a non-NULL pointer? If you really want to check whether it is cross-netns, you should use net_eq() to compare src netns with dst netns, something like: if (!net_eq(dev_net(vport->dev), dev_net(skb->dev))). Thanks. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovs: Only clear tstamp when changing namespaces
On Sun, Sep 19, 2021 at 10:44 PM Tyler Stachecki wrote: > Sorry if this is a no-op -- I'm admittedly not familiar with this part > of the tree. I had added this check based on this discussion on the > OVS mailing list: > https://mail.openvswitch.org/pipermail/ovs-discuss/2021-February/050966.html > > The motivation to add it was based on the fact that skb_scrub_packet > is doing it conditionally as well, but you seem to indicate that > skb_scrub_packet itself is already being done somewhere? I mean, skb->tstamp has been cleared when crossing netns, so: 1) you don't need to clear it again for this case; 2) clearly we fix other cases with commit 01634047bf0d, so your patch break it again. Thanks. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net v2 2/3] net/sched: flow_dissector: Fix matching on zone id for invalid conns
On Fri, Dec 10, 2021 at 8:52 PM Jakub Kicinski wrote: > > On Thu, 9 Dec 2021 09:57:33 +0200 Paul Blakey wrote: > > @@ -238,10 +239,12 @@ void > > skb_flow_dissect_ct(const struct sk_buff *skb, > > struct flow_dissector *flow_dissector, > > void *target_container, u16 *ctinfo_map, > > - size_t mapsize, bool post_ct) > > + size_t mapsize) > > { > > #if IS_ENABLED(CONFIG_NF_CONNTRACK) > > + bool post_ct = tc_skb_cb(skb)->post_ct; > > struct flow_dissector_key_ct *key; > > + u16 zone = tc_skb_cb(skb)->zone; > > enum ip_conntrack_info ctinfo; > > struct nf_conn_labels *cl; > > struct nf_conn *ct; > > @@ -260,6 +263,7 @@ skb_flow_dissect_ct(const struct sk_buff *skb, > > if (!ct) { > > key->ct_state = TCA_FLOWER_KEY_CT_FLAGS_TRACKED | > > TCA_FLOWER_KEY_CT_FLAGS_INVALID; > > + key->ct_zone = zone; > > return; > > } > > > > Why is flow dissector expecting skb cb to be TC now? > Please keep the appropriate abstractions intact. Probably because only fl_classify() calls it, but I agree with you on this point, this function is supposed to be TC independent. Thanks. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] net: openvswitch: introduce common code for flushing flows
On Mon, Aug 10, 2020 at 6:14 PM wrote: > > From: Tonghao Zhang > > To avoid some issues, for example RCU usage warning, we should > flush the flows under ovs_lock. This patch refactors > table_instance_destroy and introduces table_instance_flow_flush > which can be invoked by __dp_destroy or ovs_flow_tbl_flush. > > Signed-off-by: Tonghao Zhang Please add a Fixes tag here, I think it is probably your memory leak fix which introduced this issue. And a Reported-by, to give credits to bug reporters. Plus one minor issue below: > -static void table_instance_destroy(struct flow_table *table, > - struct table_instance *ti, > - struct table_instance *ufid_ti, > - bool deferred) > +/* Must be called with OVS mutex held. */ > +void table_instance_flow_flush(struct flow_table *table, > + struct table_instance *ti, > + struct table_instance *ufid_ti) > { > int i; > > - if (!ti) > - return; > - > - BUG_ON(!ufid_ti); > if (ti->keep_flows) > - goto skip_flows; > + return; > > for (i = 0; i < ti->n_buckets; i++) { > - struct sw_flow *flow; > struct hlist_head *head = &ti->buckets[i]; > struct hlist_node *n; > + struct sw_flow *flow; This is at most a coding style change, please do not mix coding style changes in bug fixes. You can always push coding style changes separately when net-next is open. Thanks. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] net: openvswitch: introduce common code for flushing flows
On Wed, Aug 12, 2020 at 2:59 AM wrote: > > From: Tonghao Zhang > > To avoid some issues, for example RCU usage warning and double free, > we should flush the flows under ovs_lock. This patch refactors > table_instance_destroy and introduces table_instance_flow_flush > which can be invoked by __dp_destroy or ovs_flow_tbl_flush. > > Fixes: 50b0e61b32ee ("net: openvswitch: fix possible memleak on destroy > flow-table") > Reported-by: Johan Knöös > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2020-August/050489.html > Signed-off-by: Tonghao Zhang Reviewed-by: Cong Wang Thanks. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev