[ovs-dev] [PATCH v3 1/2] ofproto-dpif-upcall: Make ukey actions modifiable with RCU.

Ethan Jackson ethan at nicira.com
Thu Aug 13 01:14:54 UTC 2015


agreed, sent a new version.

Ethan

On Wed, Aug 12, 2015 at 5:01 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>
>> On Aug 12, 2015, at 4:13 PM, Ethan J. Jackson <ethan at nicira.com> wrote:
>>
>> From: Ethan Jackson <ethan at nicira.com>
>>
>> Future patches will need to modify ukey actions in some instances.
>> This patch makes this possible by protecting them with RCU.  It also
>> adds thread safety checks to enforce the new protection mechanism.
>>
>> Signed-off-by: Ethan J. Jackson <ethan at nicira.com>
>> ---
>> ofproto/ofproto-dpif-upcall.c | 47 +++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 0f2e186..c57cebd 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -215,7 +215,6 @@ struct udpif_key {
>>     size_t key_len;                /* Length of 'key'. */
>>     const struct nlattr *mask;     /* Datapath flow mask. */
>>     size_t mask_len;               /* Length of 'mask'. */
>> -    struct ofpbuf *actions;        /* Datapath flow actions as nlattrs. */
>>     ovs_u128 ufid;                 /* Unique flow identifier. */
>>     bool ufid_present;             /* True if 'ufid' is in datapath. */
>>     uint32_t hash;                 /* Pre-computed hash for 'key'. */
>> @@ -228,6 +227,10 @@ struct udpif_key {
>>     uint64_t reval_seq OVS_GUARDED;           /* Tracks udpif->reval_seq. */
>>     bool flow_exists OVS_GUARDED;             /* Ensures flows are only deleted
>>                                                  once. */
>> +    /* Datapath flow actions as nlattrs.  Protected by RCU.  Lockless reads can
>> +     * be made with ukey_get_actions(), otherwise a lock is required. Updates
>> +     * should be made with the ukey_set_actions() function. */
>> +    const struct ofpbuf *actions OVS_GUARDED;
>>
>
> IMO it would be prudent to define this as:
>
> OVSRCU_TYPE(const struct ofpbuf *) actions;
>
> and then always use ovsrcu_get() to access it, and ovsrcu_set() to set it. This makes sure the memory barriers are there, i.e., the compiler will not rearrange any initialization of the new actions after the point in which other threads can see it.
>
> With this the OVS_GUARDED would not be needed here.
>
>   Jarno
>
>>     struct xlate_cache *xcache OVS_GUARDED;   /* Cache for xlate entries that
>>                                                * are affected by this ukey.
>> @@ -287,6 +290,8 @@ static struct udpif_key *ukey_create_from_upcall(struct upcall *,
>> static int ukey_create_from_dpif_flow(const struct udpif *,
>>                                       const struct dpif_flow *,
>>                                       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 *udpif, struct udpif_key *ukey);
>> @@ -1127,7 +1132,7 @@ process_upcall(struct udpif *udpif, struct upcall *upcall,
>>         if (upcall->sflow) {
>>             union user_action_cookie cookie;
>>             const struct nlattr *actions;
>> -            int actions_len = 0;
>> +            size_t actions_len = 0;
>>             struct dpif_sflow_actions sflow_actions;
>>             memset(&sflow_actions, 0, sizeof sflow_actions);
>>             memset(&cookie, 0, sizeof cookie);
>> @@ -1145,8 +1150,7 @@ process_upcall(struct udpif *udpif, struct upcall *upcall,
>>                 /* Lookup actions in userspace cache. */
>>                 struct udpif_key *ukey = ukey_lookup(udpif, upcall->ufid);
>>                 if (ukey) {
>> -                    actions = ukey->actions->data;
>> -                    actions_len = ukey->actions->size;
>> +                    ukey_get_actions(ukey, &actions, &actions_len);
>>                     dpif_sflow_read_actions(flow, actions, actions_len,
>>                                             &sflow_actions);
>>                 }
>> @@ -1273,8 +1277,8 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>>             op->dop.u.flow_put.mask_len = ukey->mask_len;
>>             op->dop.u.flow_put.ufid = upcall->ufid;
>>             op->dop.u.flow_put.stats = NULL;
>> -            op->dop.u.flow_put.actions = ukey->actions->data;
>> -            op->dop.u.flow_put.actions_len = ukey->actions->size;
>> +            ukey_get_actions(ukey, &op->dop.u.flow_put.actions,
>> +                             &op->dop.u.flow_put.actions_len);
>>         }
>>
>>         if (upcall->odp_actions.size) {
>> @@ -1340,6 +1344,31 @@ ukey_lookup(struct udpif *udpif, const ovs_u128 *ufid)
>>     return NULL;
>> }
>>
>> +/* Provides safe lockless access of RCU protected 'ukey->actions'.  Callers may
>> + * alternatively access the field directly if they take 'ukey->mutex'. */
>> +static void
>> +ukey_get_actions(struct udpif_key *ukey, const struct nlattr **actions, size_t *size)
>> +    OVS_NO_THREAD_SAFETY_ANALYSIS
>> +{
>> +    const struct ofpbuf *buf = ukey->actions;
>> +    *actions = buf->data;
>> +    *size = buf->size;
>> +}
>> +
>> +static void
>> +ukey_set_actions(struct udpif_key *ukey, const struct ofpbuf *actions)
>> +    OVS_REQUIRES(ukey->mutex)
>> +{
>> +    const struct ofpbuf *old_actions = ukey->actions;
>> +
>> +    ukey->actions = ofpbuf_clone(actions);
>> +
>> +    if (old_actions) {
>> +        ovsrcu_postpone(ofpbuf_delete, CONST_CAST(struct ofpbuf *,
>> +                                                  old_actions));
>> +    }
>> +}
>> +
>> static struct udpif_key *
>> ukey_create__(const struct nlattr *key, size_t key_len,
>>               const struct nlattr *mask, size_t mask_len,
>> @@ -1363,7 +1392,9 @@ ukey_create__(const struct nlattr *key, size_t key_len,
>>     ukey->ufid = *ufid;
>>     ukey->pmd_id = pmd_id;
>>     ukey->hash = get_ufid_hash(&ukey->ufid);
>> -    ukey->actions = ofpbuf_clone(actions);
>> +
>> +    ukey->actions = NULL;
>> +    ukey_set_actions(ukey, actions);
>>
>>     ovs_mutex_init(&ukey->mutex);
>>     ukey->dump_seq = dump_seq;
>> @@ -1616,7 +1647,7 @@ ukey_delete__(struct udpif_key *ukey)
>>             recirc_free_id(ukey->recircs[i]);
>>         }
>>         xlate_cache_delete(ukey->xcache);
>> -        ofpbuf_delete(ukey->actions);
>> +        ofpbuf_delete(CONST_CAST(struct ofpbuf *, ukey->actions));
>>         ovs_mutex_destroy(&ukey->mutex);
>>         free(ukey);
>>     }
>> --
>> 2.1.0
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list