On Thu, Nov 21, 2024 at 10:04:15AM +0100, Sven Eckelmann wrote:
> On Wednesday, 20 November 2024 18:47:17 CET Remi Pommarel wrote:
> > The tt.local_changes atomic is either written with tt.changes_list_lock
> > or close to it (see batadv_tt_local_event()). Thus the performance gain
> > using an atomic was limited (or because of atomic_read() impact even
> > negative). Using atomic also comes with the need to be wary of potential
> > negative tt.local_changes value.
> >
> > Simplify the tt.local_changes usage by removing the atomic property and
> > modifying it only with tt.changes_list_lock held.
>
> The overall change assumes that the compiler never splits writes (store
> tearing) [1]. Of course, writes are protected against each other using locks.
> But the reader is no longer protected from partial writes. I haven't checked
> whether store fusing might be a problem.
>
> Kind regards,
> Sven
>
> [1]
> https://www.kernel.org/doc/html/latest/core-api/wrappers/memory-barriers.html
Good catch thanks.
>
> [...]
> > @@ -783,13 +783,13 @@ static int batadv_softif_init_late(struct net_device
> > *dev)
> > atomic_set(&bat_priv->mesh_state, BATADV_MESH_INACTIVE);
> > atomic_set(&bat_priv->bcast_seqno, 1);
> > atomic_set(&bat_priv->tt.vn, 0);
> > - atomic_set(&bat_priv->tt.local_changes, 0);
> > atomic_set(&bat_priv->tt.ogm_append_cnt, 0);
> > #ifdef CONFIG_BATMAN_ADV_BLA
> > atomic_set(&bat_priv->bla.num_requests, 0);
> > #endif
> > atomic_set(&bat_priv->tp_num, 0);
> >
> > + bat_priv->tt.local_changes = 0;
>
> Would need WRITE_ONCE (just to be consistent)
>
> [...]
> > @@ -508,21 +507,17 @@ static void batadv_tt_local_event(struct batadv_priv
> > *bat_priv,
> > del:
> > list_del(&entry->list);
> > kmem_cache_free(batadv_tt_change_cache, entry);
> > + bat_priv->tt.local_changes--;
> > kmem_cache_free(batadv_tt_change_cache, tt_change_node);
> > - event_removed = true;
> > goto unlock;
> > }
> >
> > /* track the change in the OGMinterval list */
> > list_add_tail(&tt_change_node->list, &bat_priv->tt.changes_list);
> > + bat_priv->tt.local_changes++;
>
> Needs more complex constructs with WRITE_ONCE or
> __sync_add_and_fetch/__sync_sub_and_fetch (which were handled before inside
> atomic_inc). The latter are not used that often in the kernel, so I wouldn't
> want to introduce them in the batman-adv module.
What about using something in the line:
WRITE_ONCE(&bat_priv->tt.local_changes,
READ_ONCE(&bat_priv->tt.local_changes) + 1);
>
> > @@ -1022,7 +1017,7 @@ static void batadv_tt_tvlv_container_update(struct
> > batadv_priv *bat_priv)
> > tt_data->flags = BATADV_TT_OGM_DIFF;
> >
> > spin_lock_bh(&bat_priv->tt.changes_list_lock);
> > - atomic_set(&bat_priv->tt.local_changes, 0);
> > + bat_priv->tt.local_changes = 0;
> >
> > list_for_each_entry_safe(entry, safe, &bat_priv->tt.changes_list,
> > list) {
>
> Would need WRITE_ONCE
>
> > @@ -1438,7 +1433,7 @@ static void batadv_tt_changes_list_free(struct
> > batadv_priv *bat_priv)
> > kmem_cache_free(batadv_tt_change_cache, entry);
> > }
> >
> > - atomic_set(&bat_priv->tt.local_changes, 0);
> > + bat_priv->tt.local_changes = 0;
> > spin_unlock_bh(&bat_priv->tt.changes_list_lock);
> > }
>
> Would need WRITE_ONCE
Yes.
Thanks for the review.
--
Remi
