On 31 August 2016 at 13:16, Jarno Rajahalme <ja...@ovn.org> wrote:
> With one question below,
>
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
Thanks for the review,
>> On Aug 31, 2016, at 11:06 AM, Joe Stringer <j...@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 <j...@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).
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
