On Mon, May 30, 2022 at 06:01:56PM +0300, Roi Dayan wrote:
>
>
> On 2022-03-22 2:04 PM, Tao Liu wrote:
> > If netdev goes away(i.e. qemu with a vnet shutdown), kernel delete all tc
> > filters, those tcf_id related to the netdev will be left in ufid_to_tc
> > hashmap. When qemu restart with a same vnet but different ifindex assigned,
> > a dup ufid may add. Especially after hashmap_expand, the old entry will
> > insert before the new one, delete or modify tc flow will always fail.
> >
> > So delete the stale entry to avoid a duplicated ufid in hashmap.
> >
> > Fixes: dd8ca104acd7 ("netdev-tc-offloads: Don't delete ufid mapping if fail
> > to delete filter")))
> > Signed-off-by: Tao Liu <[email protected]>
> > ---
> > lib/netdev-offload-tc.c | 37 ++++++++++++++++++++++---------------
> > 1 file changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > index 12d0a9af3..e9dd66790 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -167,6 +167,9 @@ struct ufid_tc_data {
> > struct netdev *netdev;
> > };
> > +static void parse_tc_flower_to_stats(struct tc_flower *flower,
> > + struct dpif_flow_stats *stats);
> > +
> > static void
> > del_ufid_tc_mapping_unlocked(const ovs_u128 *ufid)
> > {
> > @@ -200,11 +203,24 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
> > /* Wrapper function to delete filter and ufid tc mapping */
> > static int
> > -del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid)
> > +del_filter_and_ufid_mapping(struct tcf_id *id, const ovs_u128 *ufid,
> > + struct dpif_flow_stats *stats)
> > {
> > - int err;
> > + struct tc_flower flower;
> > + int err = 0, get_err;
> > +
> > + get_err = tc_get_flower(id, &flower);
> > + if (!get_err) {
> > + err = tc_del_filter(id);
> > + }
>
> hi, beside moving the fill stats part here I'm not sure I understand
> the fix for del stale ufid. I understand instead of calling del on the
> id you check it exists first. so the result seems the same? if not,
> can you explain?
>
Hi, Roi. Sorry for late reply!
The details behind this patch were explained here:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-March/392762.html
As discussed with Eelco, watching netlink messages may be a good way to fix
this problem.
> > +
> > + if (stats) {
> > + memset(stats, 0, sizeof *stats);
> > + if (!get_err) {
> > + parse_tc_flower_to_stats(&flower, stats);
> > + }
> > + }
> > - err = tc_del_filter(id);
> > if (!err) {
> > del_ufid_tc_mapping(ufid);
> > }
> > @@ -1946,7 +1962,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct
> > match *match,
> > if (get_ufid_tc_mapping(ufid, &id) == 0) {
> > VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d",
> > id.handle, id.prio);
> > - info->tc_modify_flow_deleted = !del_filter_and_ufid_mapping(&id,
> > ufid);
> > + info->tc_modify_flow_deleted =
> > + !del_filter_and_ufid_mapping(&id, ufid, NULL);
> > }
> > prio = get_prio_for_tc_flower(&flower);
> > @@ -2017,7 +2034,6 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> > const ovs_u128 *ufid,
> > struct dpif_flow_stats *stats)
> > {
> > - struct tc_flower flower;
> > struct tcf_id id;
> > int error;
> > @@ -2026,16 +2042,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> > return error;
> > }
> > - if (stats) {
> > - memset(stats, 0, sizeof *stats);
> > - if (!tc_get_flower(&id, &flower)) {
> > - parse_tc_flower_to_stats(&flower, stats);
> > - }
> > - }
> > -
> > - error = del_filter_and_ufid_mapping(&id, ufid);
> > -
> > - return error;
> > + return del_filter_and_ufid_mapping(&id, ufid, stats);
> > }
> > static int
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
