Re: [PATCH v4 2/2] add icswx support

[Benjamin Herrenschmidt <benh@au1.ibm.com>]

On Fri, 2011-03-04 at 11:29 -0600, Tseng-Hui (Frank) Lin wrote:
> On Fri, 2011-03-04 at 12:02 +1100, Benjamin Herrenschmidt wrote:
> > On Wed, 2011-03-02 at 11:20 -0600, Tseng-Hui (Frank) Lin wrote:
> > 
> > > +#define CPU_FTR_ICSWX                  LONG_ASM_CONST(0x1000000000000000)
> > 
> > Do we want a userspace visible feature as well ? Or some other way to
> > inform userspace that we support icswx ?
> > 
> Does a user space program really need to know about icswx? Only
> coprocessor drivers need to know about icswx. Shouldn't user space
> programs talk to the coprocessor drivers instead? 

Well, I don't know how you use icswx on P7+, but on Prism it's
definitely issued directly by userspace.

> Thought about that. However, multiple threads can call use_cop() at the
> same time. Without the spinlock being setup in advance, how do I
> guarantee allocating struct copro_data and modifying the pointer in the
> mm_context to be atomic?

You don't need to. You allocate and initialize the structure, and you
compare & swap the pointer. If somebody beat you, you trash your copy. 

> > I'm not sure I totally get the point of having an ifdef here. Can't you
> > make it unconditional ? Or do you expect distros to turn that off in
> > which case what's the point ?
> > 
> There is only one coprocessor, HFI, using icswx at this moment. The lazy
> switching makes sense. However, in the future, if more types of
> coprocessors are added, the lazy switching may actually be a bad idea.
> This option allows users to turn off the lazy switching.

No user in real life plays with kernel config options. Care to explain
why the lazy switching would be a problem ?

> Same concern as above. I need something initialized in advance to
> guarantee allocating memory and updating the pointer are safe when it
> happens in use_cop().

No you don't, see above.

Cheers,
Ben.