[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 8/8] xen/arm: Add support for SMMUv3 driver



On Tue, 8 Dec 2020, Julien Grall wrote:
> On 07/12/2020 18:42, Rahul Singh wrote:
> > > On 7 Dec 2020, at 5:39 pm, Julien Grall <julien@xxxxxxx> wrote:
> > > On 07/12/2020 12:12, Rahul Singh wrote:
> > > > > > +typedef paddr_t dma_addr_t;
> > > > > > +typedef unsigned int gfp_t;
> > > > > > +
> > > > > > +#define platform_device device
> > > > > > +
> > > > > > +#define GFP_KERNEL 0
> > > > > > +
> > > > > > +/* Alias to Xen device tree helpers */
> > > > > > +#define device_node dt_device_node
> > > > > > +#define of_phandle_args dt_phandle_args
> > > > > > +#define of_device_id dt_device_match
> > > > > > +#define of_match_node dt_match_node
> > > > > > +#define of_property_read_u32(np, pname, out)
> > > > > > (!dt_property_read_u32(np, pname, out))
> > > > > > +#define of_property_read_bool dt_property_read_bool
> > > > > > +#define of_parse_phandle_with_args dt_parse_phandle_with_args
> > > > > > +
> > > > > > +/* Alias to Xen lock functions */
> > > > > > +#define mutex spinlock
> > > > > > +#define mutex_init spin_lock_init
> > > > > > +#define mutex_lock spin_lock
> > > > > > +#define mutex_unlock spin_unlock
> > > > > 
> > > > > Hmm... mutex are not spinlock. Can you explain why this is fine to
> > > > > switch to spinlock?
> > > > Yes mutex are not spinlock. As mutex is not implemented in XEN I thought
> > > > of using spinlock in place of mutex as this is the only locking
> > > > mechanism available in XEN.
> > > > Let me know if there is another blocking lock available in XEN. I will
> > > > check if we can use that.
> > > 
> > > There are no blocking lock available in Xen so far. However, if Linux were
> > > using mutex instead of spinlock, then it likely means they operations in
> > > the critical section can be long running.
> > 
> > Yes you are right Linux is using mutex when attaching a device to the SMMU
> > as this operation might take longer time.
> > > 
> > > How did you came to the conclusion that using spinlock in the SMMU driver
> > > would be fine?
> > 
> > Mutex is replaced by spinlock  in the SMMU driver when there is a request to
> > assign a device to the guest. As we are in user context at that time its ok
> > to use spinlock.
> 
> I am not sure to understand what you mean by "user context" here. Can you
> clarify it?
> 
> > As per my understanding there is one scenario when CPU will spin when there
> > is a request from the user at the same time to assign another device to the
> > SMMU and I think that is very rare.
> 
> What "user" are you referring to?
> 
> > 
> > Please suggest how we can proceed on this.
> 
> I am guessing that what you are saying is the request to assign/de-assign
> device will be issued by the toolstack and therefore they should be trusted.
> 
> My concern here is not about someone waiting on the lock to be released. It is
> more the fact that using a mutex() is an insight that the operation protected
> can be long. Depending on the length, this may result to unwanted side effect
> (e.g. other vCPU not scheduled, RCU stall in dom0, watchdog hit...).
> 
> I recall a discussion from a couple of years ago mentioning that STE
> programming operations can take quite a long time. So I would like to
> understand how long the operation is meant to last.
> 
> For a tech preview, this is probably OK to replace the mutex with an spinlock.
> But I would not want this to go past the tech preview stage without a proper
> analysis.
> 
> Stefano, what do you think?

In short, I agree.


We need to be very careful replacing mutexes with spinlocks. We need to
look closely at the ways the spinlocks could introduce unwanted
latencies. Concurrent assign_device operations are possible but rare
and, more importantly, they are user-driven so they could be mitigated.
I am more worried about other possible scenarios, e.g. STE or other
operations.

Rahul clearly put a lot of work into this series already and I think it
is better to take this incrementally, which will allow us to do better
testing and also move faster overall. So I am fine to take the series as
is now, pending an investigation on the spinlocks later.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.