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

On Sat, 2011-03-05 at 07:26 +1100, Benjamin Herrenschmidt wrote:

On Fri, 2011-03-04 at 11:29 -0600, Tseng-Hui (Frank) Lin wrote:

quoted

On Fri, 2011-03-04 at 12:02 +1100, Benjamin Herrenschmidt wrote:

quoted

On Wed, 2011-03-02 at 11:20 -0600, Tseng-Hui (Frank) Lin wrote:

quoted

+#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.

OK. You've got a point. I wasn't aware of Prism. HFI device driver is
currently the only icswx user on P7. Could you point me to more
information about how Prism uses icswx from user space?

quoted

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. 

Is atomic_cmpxchg() the one to do the trick?

quoted

quoted

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 ?

The lazy switching checks the shadow variable first before setting ACOP
register. This saves mtspr() only if the new value is the same as
current. If there are several coprocessors on the system, the ACOP
register may have to be changed frequently. In that case, the lazy
switching will not save time. In extreme case when the ACOP register
needs to be changed every time, it actually slows down the execution by
the additional shadow variable checking.

quoted

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.


_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev