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 <[email protected]> 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.
