[dpdk-dev] 回复: [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory Region cache

[Feifei Wang <Feifei.Wang2@arm.com>]

Hi, Slava

I think the second wmb can be removed. 
As I know, wmb is just a barrier to keep the order between write and write. 
and it cannot tell the CPU when it should commit the changes.

It is usually used before guard variable to keep the order that updating guard variable after
some changes, which you want to release, have been done.

For example, for the wmb  after global cache update/before altering dev_gen, it can ensure
the order that updating global cache before altering dev_gen:
1)If other agent load the changed "dev_gen", it can know the global cache has been updated.
2)If other agents load the unchanged, "dev_gen", it means the global cache has not been updated, 
and the local cache will not be flushed. 

As a result, we use  wmb and guard variable "dev_gen" to ensure the global cache updating is "visible".
The "visible" means when updating guard variable "dev_gen" is known by other agents, they also can
confirm global cache has been updated in  the meanwhile. Thus, just one wmb before altering  dev_gen
can ensure this.

Best Regards
Feifei 

> -----邮件原件-----
> 发件人: Slava Ovsiienko <viacheslavo@nvidia.com>
> 发送时间: 2021年4月20日 15:54
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>; Matan Azrad
> <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>
> 抄送: dev@dpdk.org; nd <nd@arm.com>; stable@dpdk.org; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>; nd
> <nd@arm.com>
> 主题: RE: [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory Region cache
> 
> Hi, Feifei
> 
> In my opinion, there should be 2 barriers:
>  - after global cache update/before altering dev_gen, to ensure the correct
> order
>  - after altering dev_gen to make this change visible for other agents and to
> trigger local cache update
> 
> With best regards,
> Slava
> 
> > -----Original Message-----
> > From: Feifei Wang <Feifei.Wang2@arm.com>
> > Sent: Tuesday, April 20, 2021 10:30
> > To: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> > <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>
> > Cc: dev@dpdk.org; nd <nd@arm.com>; stable@dpdk.org; Ruifeng Wang
> > <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>; nd
> > <nd@arm.com>
> > Subject: 回复: [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory
> > Region cache
> >
> > Hi, Slava
> >
> > Another question suddenly occurred to me, in order to keep the order
> > that rebuilding global cache before updating ”dev_gen“, the wmb should
> > be before updating "dev_gen" rather than after it.
> > Otherwise, in the out-of-order platforms, current order cannot be kept.
> >
> > Thus, we should change the code as:
> > a) rebuild global cache;
> > b) rte_smp_wmb();
> > c) updating dev_gen
> >
> > Best Regards
> > Feifei
> > > -----邮件原件-----
> > > 发件人: Feifei Wang
> > > 发送时间: 2021年4月20日 13:54
> > > 收件人: Slava Ovsiienko <viacheslavo@nvidia.com>; Matan Azrad
> > > <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>
> > > 抄送: dev@dpdk.org; nd <nd@arm.com>; stable@dpdk.org; Ruifeng
> Wang
> > > <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> > > 主题: 回复: [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory Region
> > > cache
> > >
> > > Hi, Slava
> > >
> > > Thanks very much for your explanation.
> > >
> > > I can understand the app can wait all mbufs are returned to the
> > > memory pool, and then it can free this mbufs, I agree with this.
> > >
> > > As a result, I will remove the bug fix patch from this series and
> > > just replace the smp barrier with C11 thread fence. Thanks very much
> > > for your patient explanation again.
> > >
> > > Best Regards
> > > Feifei
> > >
> > > > -----邮件原件-----
> > > > 发件人: Slava Ovsiienko <viacheslavo@nvidia.com>
> > > > 发送时间: 2021年4月20日 2:51
> > > > 收件人: Feifei Wang <Feifei.Wang2@arm.com>; Matan Azrad
> > > > <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>
> > > > 抄送: dev@dpdk.org; nd <nd@arm.com>; stable@dpdk.org; Ruifeng
> > Wang
> > > > <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> > > > 主题: RE: [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory Region
> > > > cache
> > > >
> > > > Hi, Feifei
> > > >
> > > > Please, see below
> > > >
> > > > ....
> > > >
> > > > > > Hi, Feifei
> > > > > >
> > > > > > Sorry, I do not follow what this patch fixes. Do we have some
> > > > > > issue/bug with MR cache in practice?
> > > > >
> > > > > This patch fixes the bug which is based on logical deduction,
> > > > > and it doesn't actually happen.
> > > > >
> > > > > >
> > > > > > Each Tx queue has its own dedicated "local" cache for MRs to
> > > > > > convert buffer address in mbufs being transmitted to LKeys
> > > > > > (HW-related entity
> > > > > > handle) and the "global" cache for all MR registered on the device.
> > > > > >
> > > > > > AFAIK, how conversion happens in datapath:
> > > > > > - check the local queue cache flush request
> > > > > > - lookup in local cache
> > > > > > - if not found:
> > > > > > - acquire lock for global cache read access
> > > > > > - lookup in global cache
> > > > > > - release lock for global cache
> > > > > >
> > > > > > How cache update on memory freeing/unregistering happens:
> > > > > > - acquire lock for global cache write access
> > > > > > - [a] remove relevant MRs from the global cache
> > > > > > - [b] set local caches flush request
> > > > > > - free global cache lock
> > > > > >
> > > > > > If I understand correctly, your patch swaps [a] and [b], and
> > > > > > local caches flush is requested earlier. What problem does it solve?
> > > > > > It is not supposed there are in datapath some mbufs
> > > > > > referencing to the memory being freed. Application must ensure
> > > > > > this and must not allocate new mbufs from this memory regions
> being freed.
> > > > > > Hence, the lookups for these MRs in caches should not occur.
> > > > >
> > > > > For your first point that, application can take charge of
> > > > > preventing MR freed memory being allocated to data path.
> > > > >
> > > > > Does it means that If there is an emergency of MR fragment, such
> > > > > as hotplug, the application must inform thedata path in advance,
> > > > > and this memory will not be allocated, and then the control path
> > > > > will free this memory? If application  can do like this, I agree
> > > > > that this bug
> > > cannot happen.
> > > >
> > > > Actually,  this is the only correct way for application to operate.
> > > > Let's suppose we have some memory area that application wants to
> free.
> > > > ALL references to this area must be removed. If we have some mbufs
> > > > allocated from this area, it means that we have memory pool
> > > > created
> > there.
> > > >
> > > > What application should do:
> > > > - notify all its components/agents the memory area is going to be
> > > > freed
> > > > - all components/agents free the mbufs they might own
> > > > - PMD might not support freeing for some mbufs (for example being
> > > > sent and awaiting for completion), so app should just wait
> > > > - wait till all mbufs are returned to the memory pool (by
> > > > monitoring available obj == pool size)
> > > >
> > > > Otherwise - it is dangerous to free the memory. There are just
> > > > some mbufs still allocated, it is regardless to buf address to MR
> > > > translation. We just can't free the memory - the mapping will be
> > > > destroyed and might cause the segmentation fault by SW or some HW
> > > > issues on DMA access to unmapped memory.  It is very generic
> > > > safety approach - do not free the memory that is still in use.
> > > > Hence, at the moment of freeing and unregistering the MR, there
> > > > MUST BE NO any
> > > mbufs in flight referencing to the addresses being freed.
> > > > No translation to MR being invalidated can happen.
> > > >
> > > > >
> > > > > > For other side, the cache flush has negative effect - the
> > > > > > local cache is getting empty and can't provide translation for
> > > > > > other valid (not being removed) MRs, and the translation has
> > > > > > to look up in the global cache, that is locked now for
> > > > > > rebuilding, this causes the delays in datapatch
> > > > > on acquiring global cache lock.
> > > > > > So, I see some potential performance impact.
> > > > >
> > > > > If above assumption is true, we can go to your second point. I
> > > > > think this is a problem of the tradeoff between cache coherence
> > > > > and
> > > > performance.
> > > > >
> > > > > I can understand your meaning that though global cache has been
> > > > > changed, we should keep the valid MR in local cache as long as
> > > > > possible to ensure the fast searching speed.
> > > > > In the meanwhile, the local cache can be rebuilt later to reduce
> > > > > its waiting time for acquiring the global cache lock.
> > > > >
> > > > > However,  this mechanism just ensures the performance unchanged
> > > > > for the first few mbufs.
> > > > > During the next mbufs lkey searching after 'dev_gen' updated, it
> > > > > is still necessary to update the local cache. And the
> > > > > performance can firstly reduce and then returns. Thus, no matter
> > > > > whether there is this patch or not,  the performance will jitter
> > > > > in a certain period of
> > time.
> > > >
> > > > Local cache should be updated to remove MRs no longer valid. But
> > > > we just flush the entire cache.
> > > > Let's suppose we have valid MR0, MR1, and not valid MRX in local cache.
> > > > And there are traffic in the datapath for MR0 and MR1, and no
> > > > traffic for MRX anymore.
> > > >
> > > > 1) If we do as you propose:
> > > > a) take a lock
> > > > b) request flush local cache first - all MR0, MR1, MRX will be
> > > > removed on translation in datapath
> > > > c) update global cache,
> > > > d) free lock
> > > > All the traffic for valid MR0, MR1 ALWAYS will be blocked on lock
> > > > taken for cache update since point b) till point d).
> > > >
> > > > 2) If we do as it is implemented now:
> > > > a) take a lock
> > > > b) update global cache
> > > > c) request flush local cache
> > > > d) free lock
> > > > The traffic MIGHT be locked ONLY for MRs non-existing in local
> > > > cache (not happens for MR0 and MR1, must not happen for MRX),  and
> > > > probability should be minor. And lock might happen since c) till
> > > > d)
> > > > - quite short period of time
> > > >
> > > > Summary, the difference between 1) and 2)
> > > >
> > > > Lock probability:
> > > > - 1) lock ALWAYS happen for ANY MR translation after b),
> > > >   2) lock MIGHT happen, for cache miss ONLY, after c)
> > > >
> > > > Lock duration:
> > > > - 1) lock since b) till d),
> > > >   2) lock since c) till d), that seems to be  much shorter.
> > > >
> > > > >
> > > > > Finally, in conclusion, I tend to think that the bottom layer
> > > > > can do more things to ensure the correct execution of the
> > > > > program, which may have a negative impact on the performance in
> > > > > a short time, but in the long run, the performance will eventually
> come back.
> > > > > Furthermore, maybe we should pay attention to the performance in
> > > > > the stable period, and try our best to ensure the correctness of
> > > > > the program in case of
> > > > emergencies.
> > > >
> > > > If we have some mbufs still allocated in memory being freed -
> > > > there is nothing to say about correctness, it is totally
> > > > incorrect. In my opinion, we should not think how to mitigate this
> > > > incorrect behavior, we should not encourage application developers
> > > > to follow the wrong
> > > approaches.
> > > >
> > > > With best regards,
> > > > Slava
> > > >
> > > > >
> > > > > Best Regards
> > > > > Feifei
> > > > >
> > > > > > With best regards,
> > > > > > Slava
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Feifei Wang <feifei.wang2@arm.com>
> > > > > > > Sent: Thursday, March 18, 2021 9:19
> > > > > > > To: Matan Azrad <matan@nvidia.com>; Shahaf Shuler
> > > > > > > <shahafs@nvidia.com>; Slava Ovsiienko
> > > > > > > <viacheslavo@nvidia.com>; Yongseok Koh <yskoh@mellanox.com>
> > > > > > > Cc: dev@dpdk.org; nd@arm.com; Feifei Wang
> > > > <feifei.wang2@arm.com>;
> > > > > > > stable@dpdk.org; Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > > Subject: [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory
> > > > > > > Region cache
> > > > > > >
> > > > > > > 'dev_gen' is a variable to inform other cores to flush their
> > > > > > > local cache when global cache is rebuilt.
> > > > > > >
> > > > > > > However, if 'dev_gen' is updated after global cache is
> > > > > > > rebuilt, other cores may load a wrong memory region lkey
> > > > > > > value from old local
> > > > > cache.
> > > > > > >
> > > > > > > Timeslot        main core               worker core
> > > > > > >   1         rebuild global cache
> > > > > > >   2                                  load unchanged dev_gen
> > > > > > >   3            update dev_gen
> > > > > > >   4                                  look up old local cache
> > > > > > >
> > > > > > > From the example above, we can see that though global cache
> > > > > > > is rebuilt, due to that dev_gen is not updated, the worker
> > > > > > > core may look up old cache table and receive a wrong memory
> > > > > > > region
> > lkey value.
> > > > > > >
> > > > > > > To fix this, updating 'dev_gen' should be moved before
> > > > > > > rebuilding global cache to inform worker cores to flush
> > > > > > > their local cache when global cache start rebuilding. And
> > > > > > > wmb can ensure the sequence of this
> > > > > > process.
> > > > > > >
> > > > > > > Fixes: 974f1e7ef146 ("net/mlx5: add new memory region
> > > > > > > support")
> > > > > > > Cc: stable@dpdk.org
> > > > > > >
> > > > > > > Suggested-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > > > > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > > > > ---
> > > > > > >  drivers/net/mlx5/mlx5_mr.c | 37
> > > > > > > +++++++++++++++++--------------------
> > > > > > >  1 file changed, 17 insertions(+), 20 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/mlx5/mlx5_mr.c
> > > > > > > b/drivers/net/mlx5/mlx5_mr.c index
> > > > > > > da4e91fc2..7ce1d3e64 100644
> > > > > > > --- a/drivers/net/mlx5/mlx5_mr.c
> > > > > > > +++ b/drivers/net/mlx5/mlx5_mr.c
> > > > > > > @@ -103,20 +103,18 @@ mlx5_mr_mem_event_free_cb(struct
> > > > > > > mlx5_dev_ctx_shared *sh,
> > > > > > >  		rebuild = 1;
> > > > > > >  	}
> > > > > > >  	if (rebuild) {
> > > > > > > -		mlx5_mr_rebuild_cache(&sh->share_cache);
> > > > > > > +		++sh->share_cache.dev_gen;
> > > > > > > +		DEBUG("broadcasting local cache flush, gen=%d",
> > > > > > > +			sh->share_cache.dev_gen);
> > > > > > > +
> > > > > > >  		/*
> > > > > > >  		 * Flush local caches by propagating invalidation
> > > across cores.
> > > > > > > -		 * rte_smp_wmb() is enough to synchronize this
> > > event. If
> > > > > > > one of
> > > > > > > -		 * freed memsegs is seen by other core, that means
> > > the
> > > > > > > memseg
> > > > > > > -		 * has been allocated by allocator, which will come
> > > after this
> > > > > > > -		 * free call. Therefore, this store instruction
> > > (incrementing
> > > > > > > -		 * generation below) will be guaranteed to be seen
> > > by other
> > > > > > > core
> > > > > > > -		 * before the core sees the newly allocated memory.
> > > > > > > +		 * rte_smp_wmb() is to keep the order that dev_gen
> > > > > > > updated before
> > > > > > > +		 * rebuilding global cache. Therefore, other core can
> > > flush
> > > > > > > their
> > > > > > > +		 * local cache on time.
> > > > > > >  		 */
> > > > > > > -		++sh->share_cache.dev_gen;
> > > > > > > -		DEBUG("broadcasting local cache flush, gen=%d",
> > > > > > > -		      sh->share_cache.dev_gen);
> > > > > > >  		rte_smp_wmb();
> > > > > > > +		mlx5_mr_rebuild_cache(&sh->share_cache);
> > > > > > >  	}
> > > > > > >  	rte_rwlock_write_unlock(&sh->share_cache.rwlock);
> > > > > > >  }
> > > > > > > @@ -407,20 +405,19 @@ mlx5_dma_unmap(struct rte_pci_device
> > > > > *pdev,
> > > > > > void
> > > > > > > *addr,
> > > > > > >  	mlx5_mr_free(mr, sh->share_cache.dereg_mr_cb);
> > > > > > >  	DEBUG("port %u remove MR(%p) from list", dev->data-
> > > >port_id,
> > > > > > >  	      (void *)mr);
> > > > > > > -	mlx5_mr_rebuild_cache(&sh->share_cache);
> > > > > > > +
> > > > > > > +	++sh->share_cache.dev_gen;
> > > > > > > +	DEBUG("broadcasting local cache flush, gen=%d",
> > > > > > > +		sh->share_cache.dev_gen);
> > > > > > > +
> > > > > > >  	/*
> > > > > > >  	 * Flush local caches by propagating invalidation across cores.
> > > > > > > -	 * rte_smp_wmb() is enough to synchronize this event. If
> > > one of
> > > > > > > -	 * freed memsegs is seen by other core, that means the
> > > memseg
> > > > > > > -	 * has been allocated by allocator, which will come after this
> > > > > > > -	 * free call. Therefore, this store instruction (incrementing
> > > > > > > -	 * generation below) will be guaranteed to be seen by other
> > > core
> > > > > > > -	 * before the core sees the newly allocated memory.
> > > > > > > +	 * rte_smp_wmb() is to keep the order that dev_gen
> > > updated
> > > > > > > before
> > > > > > > +	 * rebuilding global cache. Therefore, other core can
> > > > > > > +flush
> > > their
> > > > > > > +	 * local cache on time.
> > > > > > >  	 */
> > > > > > > -	++sh->share_cache.dev_gen;
> > > > > > > -	DEBUG("broadcasting local cache flush, gen=%d",
> > > > > > > -	      sh->share_cache.dev_gen);
> > > > > > >  	rte_smp_wmb();
> > > > > > > +	mlx5_mr_rebuild_cache(&sh->share_cache);
> > > > > > >  	rte_rwlock_read_unlock(&sh->share_cache.rwlock);
> > > > > > >  	return 0;
> > > > > > >  }
> > > > > > > --
> > > > > > > 2.25.1