[ovs-dev] [PATCHv2 2/4] upcall: Only init flow_put if ukey is installed.
Joe Stringer
joe at ovn.org
Wed Aug 31 22:12:35 UTC 2016
On 31 August 2016 at 13:16, Jarno Rajahalme <jarno at ovn.org> wrote:
> With one question below,
>
> Acked-by: Jarno Rajahalme <jarno at ovn.org>
Thanks for the review,
>> On Aug 31, 2016, at 11:06 AM, Joe Stringer <joe at 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 at 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).
More information about the dev
mailing list