diff mbox

[ovs-dev,0/6] Refactor revalidate_ukey().

Message ID CAPWQB7FdgxZhGaLzEuFiEmhN+6DQ_8qJ7iJR9u+1-BD1=7KhTQ@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Joe Stringer Sept. 28, 2016, 11:21 p.m. UTC
On 28 September 2016 at 14:22, Daniele Di Proietto <diproiettod@ovn.org> wrote:
> Thanks for doing this cleanup, my instant revalidation series should be much
> simpler after this.
>
> It looks like this needs rebasing, but it is pretty trivial
>
> I've gone through the code, I only have a couple minor of comments about how
> commits are organized.
>
> Other than that it looks good to me.
>
> For the series:
>
> Acked-by: Daniele Di Proietto <diproiettod@vmware.com>

Thanks, I'll push the series soon. The only functional change after
rebase + applying feedback is this:

               || !populate_xcache(udpif, ukey, push.tcp_flags)) {

Comments

Joe Stringer Sept. 28, 2016, 11:55 p.m. UTC | #1
On 28 September 2016 at 16:21, Joe Stringer <joe@ovn.org> wrote:
> On 28 September 2016 at 14:22, Daniele Di Proietto <diproiettod@ovn.org> wrote:
>> Thanks for doing this cleanup, my instant revalidation series should be much
>> simpler after this.
>>
>> It looks like this needs rebasing, but it is pretty trivial
>>
>> I've gone through the code, I only have a couple minor of comments about how
>> commits are organized.
>>
>> Other than that it looks good to me.
>>
>> For the series:
>>
>> Acked-by: Daniele Di Proietto <diproiettod@vmware.com>
>
> Thanks, I'll push the series soon. The only functional change after
> rebase + applying feedback is this:
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 84f1de273598..cafb46fa666b 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2035,6 +2035,10 @@ revalidate_ukey(struct udpif *udpif, struct
> udpif_key *ukey,
>             }
>             result = revalidate_ukey__(udpif, ukey, push.tcp_flags,
>                                        odp_actions, recircs, ukey->xcache);
> +            if (result == UKEY_DELETE && !ukey->xcache->entries.size) {
> +                xlate_cache_delete(ukey->xcache);
> +                ukey->xcache = NULL;
> +            }
>         } /* else delete; too expensive to revalidate */
>     } else if (!push.n_packets || ukey->xcache
>                || !populate_xcache(udpif, ukey, push.tcp_flags)) {

This part is not strictly necessary - it covers cases like failure to
do odp translation or xlate lookup. However if these fail during
revalidate_ukey__() then they're going to fail later anyway, so
allowing an empty xcache is a reasonable way to handle this case.
There's no danger of leaking the xcache since it's attached to the
ukey anyway. I'll drop this piece.
diff mbox

Patch

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 84f1de273598..cafb46fa666b 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2035,6 +2035,10 @@  revalidate_ukey(struct udpif *udpif, struct
udpif_key *ukey,
            }
            result = revalidate_ukey__(udpif, ukey, push.tcp_flags,
                                       odp_actions, recircs, ukey->xcache);
+            if (result == UKEY_DELETE && !ukey->xcache->entries.size) {
+                xlate_cache_delete(ukey->xcache);
+                ukey->xcache = NULL;
+            }
        } /* else delete; too expensive to revalidate */
    } else if (!push.n_packets || ukey->xcache