On Mon, Apr 20, 2020 at 10:28:00AM -0500, Bodong Wang wrote: > On 4/20/2020 10:15 AM, Pablo Neira Ayuso wrote: > > On Mon, Apr 20, 2020 at 09:58:10AM -0500, Bodong Wang wrote: > > [...] > > > @@ -796,6 +799,16 @@ static void flow_offload_work_stats(struct flow_offload_work *offload) > > > FLOW_OFFLOAD_DIR_REPLY, > > > stats[1].pkts, stats[1].bytes); > > > } > > > + > > > + /* Clear HW_OFFLOAD immediately when lastused stopped updating, this can > > > + * happen in two scenarios: > > > + * > > > + * 1. TC rule on a higher level device (e.g. vxlan) was offloaded, but > > > + * HW driver is unloaded. > > > + * 2. One of the shared block driver is unloaded. > > > + */ > > > + if (!lastused) > > > + clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status); > > > } > > Better inconditionally clear off the flag after the entry is removed > > from hardware instead of relying on the lastused field? > > Functionality wise, it should work. Current way is more for containing the > set/clear in the same domain, and no need to ask each vendor to take care of > this bit. No need to ask each vendor, what I mean is to deal with this from flow_offload_work_del(), see attached patch.
diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c index e3b099c14eff..593fefd52ef7 100644 --- a/net/netfilter/nf_flow_table_offload.c +++ b/net/netfilter/nf_flow_table_offload.c @@ -763,6 +763,7 @@ static void flow_offload_work_del(struct flow_offload_work *offload) flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL); flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY); set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags); + clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status); } static void flow_offload_tuple_stats(struct flow_offload_work *offload,
