[ovs-dev,PATCHv2,2/4] upcall: Only init flow_put if ukey is installed.

Currently when processing a batch of upcalls, all datapath operations
are first initialized, then later the corresponding ukeys are installed.
If the ukey_install fails at this later point, then the code needs to
backtrack a bit to delete the ukey and skip using the initialized
datapath op.

It's a little simpler to only initialize the datapath operation if the
ukey could actually be installed. The locks are held longer, but these
locks aren't heavily contended and the extended holding of the lock will
be removed in a subsequent patch anyway.

Signed-off-by: Joe Stringer <joe@ovn.org>
---
v2: First post
---
 ofproto/ofproto-dpif-upcall.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

With one question below,

Acked-by: Jarno Rajahalme <jarno@ovn.org>

> On Aug 31, 2016, at 11:06 AM, Joe Stringer <joe@ovn.org> wrote:
> 
> Currently when processing a batch of upcalls, all datapath operations
> are first initialized, then later the corresponding ukeys are installed.
> If the ukey_install fails at this later point, then the code needs to
> backtrack a bit to delete the ukey and skip using the initialized
> datapath op.
> 
> It's a little simpler to only initialize the datapath operation if the
> ukey could actually be installed. The locks are held longer, but these
> locks aren't heavily contended and the extended holding of the lock will
> be removed in a subsequent patch anyway.
> 
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
> v2: First post
> ---
> ofproto/ofproto-dpif-upcall.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index e7fcdd28c9ff..c4034f57f33e 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1337,8 +1337,10 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>         if (should_install_flow(udpif, upcall)) {
>             struct udpif_key *ukey = upcall->ukey;
> 
> -            upcall->ukey_persists = true;
> -            put_op_init(&ops[n_ops++], ukey, DPIF_FP_CREATE);
> +            if (ukey_install_start(udpif, ukey)) {
> +                upcall->ukey_persists = true;
> +                put_op_init(&ops[n_ops++], ukey, DPIF_FP_CREATE);
> +            }
>         }
> 
>         if (upcall->odp_actions.size) {
> @@ -1365,16 +1367,6 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>      */
>     n_opsp = 0;
>     for (i = 0; i < n_ops; i++) {
> -        struct udpif_key *ukey = ops[i].ukey;
> -
> -        if (ukey) {
> -            /* If we can't install the ukey, don't install the flow. */
> -            if (!ukey_install_start(udpif, ukey)) {
> -                ukey_delete__(ukey);
> -                ops[i].ukey = NULL;

Does the mean that we do not need to check key pointer for NULL somewhere else anymore?

> -                continue;
> -            }
> -        }
>         opsp[n_opsp++] = &ops[i].dop;
>     }
>     dpif_operate(udpif->dpif, opsp, n_opsp);
> -- 
> 2.9.3
>

On 31 August 2016 at 13:16, Jarno Rajahalme <jarno@ovn.org> wrote:
> With one question below,
>
> Acked-by: Jarno Rajahalme <jarno@ovn.org>

Thanks for the review,

>> On Aug 31, 2016, at 11:06 AM, Joe Stringer <joe@ovn.org> wrote:
>>
>> Currently when processing a batch of upcalls, all datapath operations
>> are first initialized, then later the corresponding ukeys are installed.
>> If the ukey_install fails at this later point, then the code needs to
>> backtrack a bit to delete the ukey and skip using the initialized
>> datapath op.
>>
>> It's a little simpler to only initialize the datapath operation if the
>> ukey could actually be installed. The locks are held longer, but these
>> locks aren't heavily contended and the extended holding of the lock will
>> be removed in a subsequent patch anyway.
>>
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>> ---
>> v2: First post
>> ---
>> ofproto/ofproto-dpif-upcall.c | 16 ++++------------
>> 1 file changed, 4 insertions(+), 12 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index e7fcdd28c9ff..c4034f57f33e 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1337,8 +1337,10 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>>         if (should_install_flow(udpif, upcall)) {
>>             struct udpif_key *ukey = upcall->ukey;
>>
>> -            upcall->ukey_persists = true;
>> -            put_op_init(&ops[n_ops++], ukey, DPIF_FP_CREATE);
>> +            if (ukey_install_start(udpif, ukey)) {
>> +                upcall->ukey_persists = true;
>> +                put_op_init(&ops[n_ops++], ukey, DPIF_FP_CREATE);
>> +            }
>>         }
>>
>>         if (upcall->odp_actions.size) {
>> @@ -1365,16 +1367,6 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>>      */
>>     n_opsp = 0;
>>     for (i = 0; i < n_ops; i++) {
>> -        struct udpif_key *ukey = ops[i].ukey;
>> -
>> -        if (ukey) {
>> -            /* If we can't install the ukey, don't install the flow. */
>> -            if (!ukey_install_start(udpif, ukey)) {
>> -                ukey_delete__(ukey);
>> -                ops[i].ukey = NULL;
>
> Does the mean that we do not need to check key pointer for NULL somewhere else anymore?

Not quite, we still may have execute actions. These do not have an
associated ukey (see just above this code in the same function).