On 31 August 2016 at 13:17, Jarno Rajahalme <ja...@ovn.org> wrote:
> With small nits below,
>
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
Thanks, I also noticed a couple of VLOGs missing their ratelimiters.
>> @@ -334,9 +345,9 @@ static int ukey_create_from_dpif_flow(const struct udpif
>> *,
>> struct udpif_key **);
>> static void ukey_get_actions(struct udpif_key *, const struct nlattr
>> **actions,
>> size_t *size);
>> -static bool ukey_install_start(struct udpif *, struct udpif_key *ukey);
>> -static bool ukey_install_finish(struct udpif_key *ukey, int error);
>> +static bool ukey_install__(struct udpif *, struct udpif_key *ukey);
>
> You need OVS_TRY_LOCK(true, ukey->mutex) here as, and in general all
> declarations should have the same thread safety annotations as the
> definitions themselves.
OK. I wasn't sure whether this was necessary.
>> static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
>> +static void transition_ukey(struct udpif_key *, enum ukey_state dat);
>
> You need OVS_REQUIRES(ukey->mutex) here for thread safety analysis to be done.
Ack, will update.
>> @@ -1359,20 +1372,23 @@ handle_upcalls(struct udpif *udpif, struct upcall
>> *upcalls,
>> }
>> }
>>
>> - /* Execute batch.
>> - *
>> - * We install ukeys before installing the flows, locking them for
>> exclusive
>> - * access by this thread for the period of installation. This ensures
>> that
>> - * other threads won't attempt to delete the flows as we are creating
>> them.
>> - */
>> + /* Execute batch. */
>> n_opsp = 0;
>> for (i = 0; i < n_ops; i++) {
>> opsp[n_opsp++] = &ops[i].dop;
>> }
>> dpif_operate(udpif->dpif, opsp, n_opsp);
>> for (i = 0; i < n_ops; i++) {
>> - if (ops[i].ukey) {
>> - ukey_install_finish(ops[i].ukey, ops[i].dop.error);
>> + struct udpif_key *ukey = ops[i].ukey;
>> +
>> + if (ukey) {
>> + if (ops[i].dop.error) {
>> + transition_ukey(ukey, UKEY_EVICTED);
>> + } else {
>> + ovs_mutex_lock(&ukey->mutex);
>> + transition_ukey(ukey, UKEY_OPERATIONAL);
>> + ovs_mutex_unlock(&ukey->mutex);
>> + }
>
> Upper transition_ukey() requires the mutex as well.
True. I was thinking that the upper was still in UKEY_CREATED state,
but it is actually UKEY_VISIBLE which means someone else could look at
it. I'll update this.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
