On Tue, Feb 12, 2019 at 10:40:30AM +0300, Ilya Maximets wrote:
> On 11.02.2019 22:15, Aaron Conole wrote:
> > Ilya Maximets <i.maxim...@samsung.com> writes:
> >
> >> All accesses to 'pmd->poll_list' should be guarded by
> >> 'pmd->port_mutex'. Additionally fixed inappropriate usage
> >> of 'HMAP_FOR_EACH_SAFE' (hmap doesn't change in a loop)
> >> and dropped not needed local variable 'proc_cycles'.
> >>
> >> CC: Nitin Katiyar <nitin.kati...@ericsson.com>
> >> Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
> >> Signed-off-by: Ilya Maximets <i.maxim...@samsung.com>
> >> ---
> >> lib/dpif-netdev.c | 14 +++++++++-----
> >> 1 file changed, 9 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> >> index 4d5bc576a..914b2bb8b 100644
> >> --- a/lib/dpif-netdev.c
> >> +++ b/lib/dpif-netdev.c
> >> @@ -3776,9 +3776,12 @@ set_pmd_auto_lb(struct dp_netdev *dp)
> >> continue;
> >> }
> >>
> >> + ovs_mutex_lock(&pmd->port_mutex);
> >> if (hmap_count(&pmd->poll_list) > 1) {
> >> multi_rxq = true;
> >> }
> >> + ovs_mutex_unlock(&pmd->port_mutex);
> >
> > What does this mutex provide here? I ask because as soon as we unlock,
> > the result is no longer assured, and we've used it to inform the
> > multi_rxq value.
> >
> > Are you afraid that the read is non-atomic so you'll end up with the
> > multi_rxq condition true, even when it should not be? That can happen
> > anyway - as soon as we unlock the count can change.
> >
> > Maybe there's a better change to support that, where hmap_count does an
> > atomic read, or there's a version that can guarantee a full load with
> > no intervening writes. But I don't see
> >
> > Maybe the pmd object lifetime goes away... but that wouldn't make sense,
> > because then the port_mutex itself would be invalid.
> >
> > I'm confused what it does to add a lock here for either the safety, or
> > the logic. I probably missed something. Or maybe it's just trying to
> > be safe in case the hmap implementation would change in the future? I
> > think if that's the case, there might be an alternate to implement?
>
> The mutex here is mostly for a code consistency. We're marking the 'poll_list'
> as OVS_GUARDED. This means that we should guard it. Unfortunately, unlike ะก++,
> in C we can not use OVS_GUARDED_BY(port_mutex) inside the structure and
> clang could not track what metex should protect the variable. I agree that
> we're
> not really need to protect this. Furthermore, the only writer for 'poll_list'
> is the main thread and 'set_pmd_auto_lb' is also called only in a main thread.
> So we don't need to protect any read accesses.
> However, for the safety, these details should not be taken into account to not
> provide code snippets that could be copied to the other (less safe) places and
> produce issues there. Especially because clang can't protect us from this kind
> of mistakes. Code architecture also could be changed in the future.
> As a developer I'd like to always assume that any access to hmap is not thread
> safe regardless of its internal implementation. Just like I'm assuming that
> cmap is safe for readers.
> This is affordable since we're not on a hot path.
Maybe a comment would be helpful.
/* We don't really need this lock but it suppresses a Clang warning. */
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
