Re: [dpdk-stable] [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory Region cache

[Slava Ovsiienko <viacheslavo@nvidia.com>]

Hi, Feifei

.. snip..
> 
> I can understand you worry that if there is no 'wmb at last', when agent_1
> leave the locked section, agent_2 still may observe unchanged global cache.
> 
> However, when agent_2 take a lock and get(new MR) in time slot 7(Fig.1), it
> means
> agent_1 has finished updating global cache and lock is freed.
> Besides, if agent_2 can take a lock, it also shows agent_2 has observed the
> changed global cache.
> 
> This is because there is a store- release in rte_rwlock_read_unlock,  store-
> release ensures all store operation before 'release' can be committed  if
> store operation after 'release' is observed by other agents.

OK, I missed this implicit ordering hidden in the lock/unlock(), thank you for
pointing me out that.

I checked the listings for rte_rwlock_write_unlock(), it is implemented
with __atomic_store_n(&rwl->cnt, 0, __ATOMIC_RELEASE) and:
- on x86 there is nothing special in the code due to strict x86-64 ordering model
- on ARMv8 there is stlxr instruction, that implies the write barrier

Now you convinced me 😊, we can get rid of the explicit "wmb_at_last" in the code.
Please, provide the patch with clear commit message with details above,
it is important to mention:
- lock protection is involved, dev_gen and cache update ordering inside the protected section does not matter
- unlock() provides implicit write barrier due to atomic operation  _ATOMIC_RELEASE

Also, in my opinion, there should be small comment in place of wmb being revomed, 
reminding we are in the protected section and unlock provides the implicit write barrier
at the level visible by software.

Thank you for the meaningful discussion, I refreshed my knowledge of memory ordering models
for different architectures 😊

With best regards,
Slava

> >
> > >
> > > Best Regards
> > > Feifei
> > >
> > > > With best regards,
> > > > Slava
> > > >
> > > >
> > > > >
> > > > > Best Regards
> > > > > Feifei
> > > > >
> > > > > > -----邮件原件-----
> > > > > > 发件人: Slava Ovsiienko <viacheslavo@nvidia.com>
> > > > > > 发送时间: 2021年5月7日 18:15
> > > > > > 收件人: 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
> > > > > >
> > > > > > We should consider the locks in your scenario - it is crucial
> > > > > > for the complete model description:
> > > > > >
> > > > > > How agent_1 (in your terms) rebuilds global cache:
> > > > > >
> > > > > > 1a) lock()
> > > > > > 1b) rebuild(global cache)
> > > > > > 1c) update(dev_gen)
> > > > > > 1d) wmb()
> > > > > > 1e) unlock()
> > > > > >
> > > > > > How agent_2 checks:
> > > > > >
> > > > > > 2a) check(dev_gen) (assume positive - changed)
> > > > > > 2b) clear(local_cache)
> > > > > > 2c) miss(on empty local_cache) - > eventually it goes to
> > > > > > mr_lookup_caches()
> > > > > > 2d) lock()
> > > > > > 2e) get(new MR)
> > > > > > 2f) unlock()
> > > > > > 2g) update(local cache with obtained new MR)
> > > > > >
> > > > > > Hence, even if 1c) becomes visible in 2a) before 1b) committed
> > > > > > (say, due to out-of-order Arch) - the agent 2 would be blocked
> > > > > > on
> > > > > > 2d) and scenario depicted on your Fig2 would not happen
> > > > > > (agent_2 will wait before step 3 till agent 1 unlocks after its step 5).
> > > > > >
> > > > > > With best regards,
> > > > > > Slava
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Feifei Wang <Feifei.Wang2@arm.com>
> > > > > > > Sent: Friday, May 7, 2021 9:36> 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>
> > > > > > > Subject: 回复: [PATCH v1 3/4] net/mlx5: fix rebuild bug for
> > > > > > > Memory Region cache
> > > > > > >
> > > > > > > Hi, Slava
> > > > > > >
> > > > > > > Thanks very much for your reply.
> > > > > > >
> > > > > > > > -----邮件原件-----
> > > > > > > > 发件人: Slava Ovsiienko <viacheslavo@nvidia.com>
> > > > > > > > 发送时间: 2021年5月6日 19:22
> > > > > > > > 收件人: 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
> > > > > > > >
> > > > > > > > Sorry, I do not follow why we should get rid of the last
> > > > > > > > (after dev_gen update) wmb.
> > > > > > > > We've rebuilt the global cache, we should notify other
> > > > > > > > agents it's happened and they should flush local caches.
> > > > > > > > So, dev_gen change should be made visible to other agents
> > > > > > > > to trigger this activity and the second wmb is here to ensure this.
> > > > > > >
> > > > > > > 1. For the first problem why we should get rid of the last
> > > > > > > wmb and move it before dev_gen updated, I think our
> > > > > > > attention is how the wmb implements the synchronization
> > > > > > > between multiple
> > agents.
> > > > > > > 					Fig1
> > > > > > > ------------------------------------------------------------
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > ------------------------
> > > > > > > -------
> > > > > > > Timeslot        		agent_1               		   agent_2
> > > > > > > 1          		rebuild global cache
> > > > > > > 2                       		wmb
> > > > > > > 3            		     update dev_gen ----------------------- load
> > changed
> > > > > > > dev_gen
> > > > > > > 4                                  			        	           rebuild local
> > > cache
> > > > > > > ------------------------------------------------------------
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > ------------------------
> > > > > > > -------
> > > > > > >
> > > > > > > First, wmb is only for local thread to keep the order
> > > > > > > between local
> > > > > > > write- write :
> > > > > > > Based on the picture above, for agent_1, wmb keeps the order
> > > > > > > that rebuilding global cache is always before updating dev_gen.
> > > > > > >
> > > > > > > Second, agent_1 communicates with agent_2 by the global
> > > > > > > variable "dev_gen" :
> > > > > > > If agent_1 updates dev_gen, agent_2 will load it and then it
> > > > > > > knows it should rebuild local cache
> > > > > > >
> > > > > > > Finally, agent_2 rebuilds local cache according to whether
> > > > > > > agent_1 has rebuilt global cache, and agent_2 knows this
> > > > > > > information by the variable
> > > > > > "dev_gen".
> > > > > > > 					Fig2
> > > > > > > ------------------------------------------------------------
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > ------------------------
> > > > > > > -------
> > > > > > > Timeslot        		agent_1               		   agent_2
> > > > > > > 1		        update dev_gen
> > > > > > > 2						      load changed
> > > dev_gen
> > > > > > > 3						          rebuild local
> > > cache
> > > > > > > 4        		    rebuild global cache
> > > > > > > 5			 wmb
> > > > > > > ------------------------------------------------------------
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > --
> > > > > > > ------------------------
> > > > > > > -------
> > > > > > >
> > > > > > > However, in arm platform, if wmb is after dev_gen updated,
> > > "dev_gen"
> > > > > > > may be updated before agent_1 rebuilding global cache, then
> > > > > > > agent_2 maybe receive error message and rebuild its local
> > > > > > > cache in
> > > > advance.
> > > > > > >
> > > > > > > To summarize, it is not important which time other agents
> > > > > > > can see the changed global variable "dev_gen".
> > > > > > > (Actually, wmb after "dev_gen" cannot ensure changed
> "dev_gen"
> > > > > > > is committed to the global).
> > > > > > > It is more important that if other agents see the changed
> > > > > > > "dev_gen", they also can know global cache has been updated.
> > > > > > >
> > > > > > > > One more point, due to registering new/destroying existing
> > > > > > > > MR involves FW (via kernel) calls, it takes so many CPU
> > > > > > > > cycles that we could neglect wmb overhead at all.
> > > > > > >
> > > > > > > We just move the last wmb into the right place, and not
> > > > > > > delete it for performance.
> > > > > > >
> > > > > > > >
> > > > > > > > Also, regarding this:
> > > > > > > >
> > > > > > > >  > > 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.
> > > > > > > >
> > > > > > > > it is not clear why ordering is important - global cache
> > > > > > > > update and dev_gen change happen under spinlock
> > > > > > > > protection, so only the last wmb is meaningful.
> > > > > > > >
> > > > > > >
> > > > > > > 2. The second function of wmb before "dev_gen" updated is
> > > > > > > for performance according to our previous discussion.
> > > > > > > According to Fig2, if there is no wmb between "global cache
> > updated"
> > > > > > > and "dev_gen updated", "dev_gen" may update before global
> > > > > > > cache
> > > > > > updated.
> > > > > > >
> > > > > > > Then agent_2 may see the changed "dev_gen" and flush entire
> > > > > > > local cache in advance.
> > > > > > >
> > > > > > > This entire flush can degrade the performance:
> > > > > > > "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 data
> > > > > > > path on acquiring global
> > > cache lock."
> > > > > > >
> > > > > > > Furthermore, spinlock is just for global cache, not for
> > > > > > > dev_gen and local cache.
> > > > > > >
> > > > > > > > To summarize, in my opinion:
> > > > > > > > - if you see some issue with ordering of global cache
> > > > > > > > update/dev_gen signalling,
> > > > > > > >   could you, please, elaborate? I'm not sure we should
> > > > > > > > maintain an order (due to spinlock protection)
> > > > > > > > - the last rte_smp_wmb() after dev_gen incrementing should
> > > > > > > > be kept intact
> > > > > > > >
> > > > > > >
> > > > > > > At last, for my view, there are two functions that moving
> > > > > > > wmb before "dev_gen"
> > > > > > > for the write-write order:
> > > > > > > --------------------------------
> > > > > > > a) rebuild global cache;
> > > > > > > b) rte_smp_wmb();
> > > > > > > c) updating dev_gen
> > > > > > > -------------------------------- 1. Achieve synchronization
> > > > > > > between multiple threads in the right way 2.
> > > > > > > Prevent other agents from flushing local cache early to
> > > > > > > ensure performance
> > > > > > >
> > > > > > > Best Regards
> > > > > > > Feifei
> > > > > > >
> > > > > > > > With best regards,
> > > > > > > > Slava
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Feifei Wang <Feifei.Wang2@arm.com>
> > > > > > > > > Sent: Thursday, May 6, 2021 5:52
> > > > > > > > > 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>
> > > > > > > > > Subject: 回复: [PATCH v1 3/4] net/mlx5: fix rebuild bug
> > > > > > > > > for Memory Region cache
> > > > > > > > >
> > > > > > > > > Hi, Slava
> > > > > > > > >
> > > > > > > > > Would you have more comments about this patch?
> > > > > > > > > For my sight, only one wmb before "dev_gen" updating is
> > > > > > > > > enough to synchronize.
> > > > > > > > >
> > > > > > > > > Thanks very much for your attention.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Best Regards
> > > > > > > > > Feifei
> > > > > > > > >
> > > > > > > > > > -----邮件原件-----
> > > > > > > > > > 发件人: Feifei Wang
> > > > > > > > > > 发送时间: 2021年4月20日 16:42
> > > > > > > > > > 收件人: 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>
> > > > > > > > > > 主题: 回复: [PATCH v1 3/4] net/mlx5: fix rebuild bug for
> > > > > > > > > > Memory
> > > > > > > Region
> > > > > > > > > > cache
> > > > > > > > > >
> > > > > > > > > > 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