It could be I am still missing something here. It seems most of the
writes are protected by the write lock, with the exception
of bond_recirculation_account(), which is the main issue this patch
fixes. So would it be simpler to just make it acquire the write lock
instead of read lock? Unless it is changed recently, my understanding
is that only the run loops that runs in the main thread will access
the bond
stats.
On Sun, Apr 13, 2014 at 10:30 PM, Joe Stringer <j...@wand.net.nz> wrote:
> From: Joe Stringer <joestrin...@nicira.com>
>
> dcf00ba35a0 (ofproto/bond: Implement bond megaflow using recirculation)
> added bond account functions that protected the statistics with only
> readlocks. This patch adds a mutex to ensure that they are updated
> correctly, and adds threadsafety annotations to the relevant functions.
>
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> ---
> ofproto/bond.c | 77
> ++++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 67 insertions(+), 10 deletions(-)
>
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index 8554955..49cef0b 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -62,14 +62,20 @@ static struct hmap *const all_bonds
> OVS_GUARDED_BY(rwlock) = &all_bonds__;
> * "struct bond" has an array of BOND_BUCKETS of these. */
> struct bond_entry {
> struct bond_slave *slave; /* Assigned slave, NULL if unassigned. */
> - uint64_t tx_bytes; /* Count of bytes recently transmitted. */
> struct list list_node; /* In bond_slave's 'entries' list. */
> -
> - /* Recirculation. */
> struct rule *pr_rule; /* Post recirculation rule for this entry.*/
> - uint64_t pr_tx_bytes; /* Record the rule tx_bytes to figure out
> - the delta to update the tx_bytes entry
> - above.*/
> +
> + /* Bond Statistics.
> + *
> + * Any reader or writer of 'tx_bytes' or 'pr_tx_bytes' must hold 'rwlock'
> + * to prevent the bond_entry from disappearing.
> + * Readers and writers with a readlock on 'rwlock' must also acquire
> + * 'mutex' before reading or modifying the statistics. */
> + struct ovs_mutex mutex OVS_ACQ_AFTER(rwlock);
> + uint64_t tx_bytes OVS_GUARDED; /* Count of bytes recently
> transmitted.*/
> + uint64_t pr_tx_bytes OVS_GUARDED; /* Record the rule tx_bytes to figure
> out
> + the delta to update the tx_bytes
> entry
> + above. */
> };
>
> /* A bond slave, that is, one of the links comprising a bond. */
> @@ -151,7 +157,12 @@ struct bond_pr_rule_op {
> struct rule *pr_rule;
> };
>
> +static void bond_entries_init(struct bond_entry **entryp)
> + OVS_REQ_WRLOCK(rwlock);
> static void bond_entry_reset(struct bond *) OVS_REQ_WRLOCK(rwlock);
> +static void bond_entries_uninit__(struct bond_entry **entryp)
> + OVS_REQ_WRLOCK(rwlock);
> +static void bond_entries_uninit(struct bond_entry **entryp);
> static struct bond_slave *bond_slave_lookup(struct bond *, const void
> *slave_)
> OVS_REQ_RDLOCK(rwlock);
> static void bond_enable_slave(struct bond_slave *, bool enable)
> @@ -271,7 +282,7 @@ bond_unref(struct bond *bond)
> hmap_destroy(&bond->slaves);
>
> ovs_mutex_destroy(&bond->mutex);
> - free(bond->hash);
> + bond_entries_uninit(&bond->hash);
> free(bond->name);
>
> HMAP_FOR_EACH_SAFE(pr_op, next_op, hmap_node, &bond->pr_rule_ops) {
> @@ -859,9 +870,11 @@ bond_entry_account(struct bond_entry *entry, uint64_t
> rule_tx_bytes)
> if (entry->slave) {
> uint64_t delta;
>
> + ovs_mutex_lock(&entry->mutex);
> delta = rule_tx_bytes - entry->pr_tx_bytes;
> entry->tx_bytes += delta;
> entry->pr_tx_bytes = rule_tx_bytes;
> + ovs_mutex_unlock(&entry->mutex);
> }
> }
>
> @@ -958,6 +971,7 @@ bond_slave_from_bal_node(struct list *bal)
> OVS_REQ_RDLOCK(rwlock)
>
> static void
> log_bals(struct bond *bond, const struct list *bals)
> + OVS_REQ_RDLOCK(rwlock)
> {
> if (VLOG_IS_DBG_ENABLED()) {
> struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -981,8 +995,10 @@ log_bals(struct bond *bond, const struct list *bals)
> if (&e->list_node != list_front(&slave->entries)) {
> ds_put_cstr(&ds, " + ");
> }
> + ovs_mutex_lock(&e->mutex);
> ds_put_format(&ds, "h%"PRIdPTR": %"PRIu64"kB",
> e - bond->hash, e->tx_bytes / 1024);
> + ovs_mutex_unlock(&e->mutex);
> }
> ds_put_cstr(&ds, ")");
> }
> @@ -995,6 +1011,7 @@ log_bals(struct bond *bond, const struct list *bals)
> /* Shifts 'hash' from its current slave to 'to'. */
> static void
> bond_shift_load(struct bond_entry *hash, struct bond_slave *to)
> + OVS_REQ_WRLOCK(rwlock)
> {
> struct bond_slave *from = hash->slave;
> struct bond *bond = from->bond;
> @@ -1026,6 +1043,7 @@ bond_shift_load(struct bond_entry *hash, struct
> bond_slave *to)
> * shift away small hashes or large hashes. */
> static struct bond_entry *
> choose_entry_to_migrate(const struct bond_slave *from, uint64_t to_tx_bytes)
> + OVS_REQ_WRLOCK(rwlock)
> {
> struct bond_entry *e;
>
> @@ -1567,20 +1585,59 @@ bond_init(void)
> }
>
> static void
> +bond_entries_init(struct bond_entry **entryp)
> +{
> + int i;
> + size_t hash_len;
> + struct bond_entry *e;
> +
> + hash_len = BOND_BUCKETS * sizeof **entryp;
> + *entryp = e = xmalloc(hash_len);
> + for (i = 0; i < BOND_BUCKETS; i++) {
> + ovs_mutex_init(&e[i].mutex);
> + }
> +}
> +
> +static void
> +bond_entries_uninit__(struct bond_entry **entryp)
> +{
> + int i;
> + struct bond_entry *e;
> +
> + if (!*entryp) {
> + return;
> + }
> +
> + e = *entryp;
> + for (i = 0; i < BOND_BUCKETS; i++) {
> + ovs_mutex_destroy(&e[i].mutex);
> + }
> + free(*entryp);
> + *entryp = NULL;
> +}
> +
> +static void
> +bond_entries_uninit(struct bond_entry **entryp)
> +{
> + ovs_rwlock_wrlock(&rwlock);
> + bond_entries_uninit__(entryp);
> + ovs_rwlock_unlock(&rwlock);
> +}
> +
> +static void
> bond_entry_reset(struct bond *bond)
> {
> if (bond->balance != BM_AB) {
> size_t hash_len = BOND_BUCKETS * sizeof *bond->hash;
>
> if (!bond->hash) {
> - bond->hash = xmalloc(hash_len);
> + bond_entries_init(&bond->hash);
> }
> memset(bond->hash, 0, hash_len);
>
> bond->next_rebalance = time_msec() + bond->rebalance_interval;
> } else {
> - free(bond->hash);
> - bond->hash = NULL;
> + bond_entries_uninit__(&bond->hash);
> }
> }
>
> --
> 1.7.10.4
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
