agreed, sent a new version.
Ethan
On Wed, Aug 12, 2015 at 5:01 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>
>> On Aug 12, 2015, at 4:13 PM, Ethan J. Jackson <et...@nicira.com> wrote:
>>
>> From: Ethan Jackson <et...@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 <et...@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@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
