On Thursday 27 May 2010, Chris Metcalf wrote: > > Yes, that makes sense. You definitely want binary compatibility between > > 32 bit binaries from a native 32 bit system on TILE-Gx in the syscall > > interface. > > > > The thing is, the COMPAT layer on TILE-Gx is actually not providing > TILEPro compatibility, since the architectures are too different -- > conceptually similar but with different opcode numbering, etc. Instead > what it's doing is providing a 32-bit pointer ABI, to help porting > crufty old code (this is in fact the primary customer driver), or to > allow more compact representations of pointer-heavy data. Ah, interesting. I don't think any architecture does it this way so far. IIRC, while alpha had some applications built in 32 bit mode in the early days, those were just using the 64 bit system calls directly. Then again, that probably required some rather ugly hacks to get the libc working, so since we have the compat code in kernel now, your approach is probably much better. Are you able to build 32 bit kernels for TILE-Gx as well? It's probably something you never really want to do for performance reasons, but I guess you could use that to verify that the ABI is really compatible. > > compat_sys_sendfile will not be needed with the asm-generic/unistd.h definitions, > > but I think you will still need a compat_sys_sendfile64, to which the same > > applies as to compat_sys_sched_rr_get_interval. > > > > I think it's the other way around: compat_sys_sendfile64() is just > sys_sendfile64(), but compat_sys_sendfile() needs to exist since it has > to write a 32-bit pointer back to userspace. Ah. I guess you're right about compat_sys_sendfile64 not being needed. Funny enough, parisc, powerpc, s390 and sparc all define it anyway, so it didn't occur to me that they don't actually need to. What I meant about compat_sys_sendfile is that you only define it if the 32 bit ABI contains a reference to sys_sendfile in the first place. With asm-generic/unistd.h, 32 bit uses always uses the sys_sendfile64 kernel interface, while for 64 bit the two are identical, so we take the regular sys_sendfile. > >> +static int hardwall_ioctl(struct inode *inode, struct file *file, > >> + unsigned int a, unsigned long b) > >> +{ > >> [...] > >> > > The hardwall stuff looks like it is quite central to your architecture. > > Could you elaborate on what it does? > > > It's not "central" but it is an important enabler for access to our > "user network". This is a wormhole-routed mesh network (the UDN, or > user dynamic network) that connects all the cpus. If a task affinitizes > itself to a single cpu (to avoid migration) and opens /dev/hardwall and > does an ioctl on it, it can associate the particular /dev/hardwall file > object with some non-overlapping subrectangle of the whole 8x8 chip (our > cpus are laid out as "tiles" in an 8x8 configuration). It can then do > an "activate" ioctl to get access to that subrectangle of the UDN, from > that cpu. Other threads in that process (or anyone who can share that > file object one way or another, e.g. fork or sendmsg) can then also do > an "activate" ioctl on that file object and also get access, and they > can then exchange messages with very low latency (register file to > register file in a handful of cycles) and high bandwidth (32 bits/cycle > or about 3GB/sec). > > The actual "hardwall" refers to the fact that cpus on the periphery of > the allocated subrectangle of cpus set up the router so that they will > get an interrupt if some cpu tries to send a message that would > terminate outside the set of allocated cpus. Doing it this way means > several unrelated tasks could have separate message-passing arenas > (spatially dividing the chip) and whenever the last task holding a > reference to a hardwall file object exits, the OS can drain any messages > from the UDN and deallocate the subrectangle in question. > > > If it is as essential as it looks, I'd vote for promoting the interface > > from an ioctl based one to four real system calls (more if necessary). > > > > The notion of using a file descriptor as the "rights" object is pretty > central, so I think a character device will work out well. ok, I see. Now you could easily do this with system calls as well: Instead of the initial ioctl that associates the file descriptor with a rectangle, you can have a syscall that creates a rectangle and a file descriptor (using anon_inode_getfd) associated with it, and returns the fd to user space. This is similar to what we do for other system call interfaces that operate on their own fds. Another alternative might be to combine this with cpusets subsystem, which has a related functionality. I guess that would be the preferred way if you expect tile-gx to take over the world and have lots of applications written to it. For a niche product, the syscall or ioctl approach does seem simple enough, and it does not require other users of cpusets to learn about requirements of your rectangles. > > Note that the procfs file format is part of your ABI, and this looks > > relatively hard to parse, which may introduce bugs. > > For per-process information, it would be better to have a simpler > > file in each /proc/<pid>/directory. Would that work for you? > > > > Well, the hardwalls aren't exactly per-process anyway, and we don't in > practice use the ASCII output for anything much, so it may not matter > that they're not too parseable. I may just look into making them more > parsable when I convert it to a /dev interface and leave it at that. On a chardev, a binary interface seems more appropriate than an text based one anyway, so you could add another ioctl for this. > I'm planning to defer this in any case, since the UDN interface, though > a nice-to-have, obviously isn't needed to run any standard C code. I'll > make that part of a follow-up patch. ok > > Note that we're about to remove the .ioctl file operation and > > replace it with .unlocked_ioctl everywhere. > > > > OK, for now I'll ensure that we are locking everything internally > correctly. I believe we are already anyway. ok. Then please just use .unlocked_ioctl in new drivers. > > [hugevmap] Not used anywhere apparently. Can you explain what this is good for? > > Maybe leave it out for now, until you merge the code that needs it. > > I don't see anything obviously wrong with the implementation though. > > > > I'll omit it; we haven't used it yet. The intent was to provide > guaranteed huge pages for TLB purposes to kernel drivers. Currently we > just start with huge pages where possible, and fragment them if necessary. Ok. Do you use huge pages for backing the linear kernel mapping? Normally device drivers get huge pages for free in kmalloc and get_free_pages because all the memory is mapped using the largest page size anyway. > > +EXPORT_SYMBOL(inb); > > > > If you just remove these definitions, you get a link error for any > > driver that tries to use these, which is probably more helpful than > > the panic. > > > > OTOH, are you sure that you can't just map the PIO calls to mmio functions > > like readb plus some fixed offset? On most non-x86 architectures, the PIO > > area of the PCI bus is just mapped to a memory range somewhere. > > > > I'll try to remove them and see if anything falls over. We don't have > any memory-mapped addresses in the 32-bit architecture, though that > changes with the 64-bit architecture, which introduces IO mappings. For > PCI we actually have to do a hypervisor transaction for reads or writes. Ok, then I assume that PIO would also be a hypervisor call, right? If you don't have MMIO on 32 bit, you might want to not define either PIO (inb, ...) no MMIO (readb, ...) calls there and disable CONFIG_HAVE_MMIO in Kconfig. > >> +SEQ_PROC_ENTRY(interrupts) > >> +static int proc_tile_interrupts_show(struct seq_file *sf, void *v) > >> +{ > >> [...] > >> > > Can you merge this with /proc/interrupts? > > > > It turns out /proc/interrupts is formatted the wrong way round if you > have 64 processors :-) You want one row per cpu, not one column per cpu! Yes, interesting observation. I'm sure the Altix folks are suffering from this a lot. > Also, there are things listed that are not strictly IRQs in the normal > sense (things like TLB flushes and syscalls) which are still good for > assessing where latency glitches might be coming from on a particular cpu. That's fine, just look at what a current x86 kernel gives you (slightly cut): CPU0 CPU1 0: 18764948 504980 IO-APIC-edge timer 1: 228456 2572 IO-APIC-edge i8042 9: 2632595 79544 IO-APIC-fasteoi acpi 12: 1094468 43409 IO-APIC-edge i8042 16: 82761 1455 IO-APIC-fasteoi uhci_hcd:usb6, yenta, heci 28: 908865 85857 PCI-MSI-edge ahci 29: 6421 11595 PCI-MSI-edge eth0 NMI: 0 0 Non-maskable interrupts LOC: 1987682 9057144 Local timer interrupts SPU: 0 0 Spurious interrupts CNT: 0 0 Performance counter interrupts PND: 0 0 Performance pending work RES: 3598785 3903513 Rescheduling interrupts CAL: 8848 5944 Function call interrupts TLB: 31467 18283 TLB shootdowns TRM: 0 0 Thermal event interrupts THR: 0 0 Threshold APIC interrupts MCE: 0 0 Machine check exceptions MCP: 354 346 Machine check polls ERR: 0 MIS: 0 Lots of things in there that fit your category. > > This seems to be read-only and coming from a kernel command > > line option, so I guess looking at /proc/cmdline would > > be a reasonable alternative. > > > > I always find that kind of painful, since you have to parse it exactly > as the kernel would to be sure you're getting it right; strstr() is only > a 99% solution. How about making it a module_param then? You can still see it in /sys/modules/*/parameters then, even if the code is builtin, but it won't be in the sysctl name space any more. > >> +SYSCALL_DEFINE3(raise_fpe, int, code, unsigned long, addr, > >> + struct pt_regs *, regs) > >> > > Does this need to be a system call? I thought we already had > > other architectures without floating point exceptions in hardware > > that don't need this. > > > > Hmm, I didn't know about that. Any information would be appreciated. I > guess you could synthesize something that looked like a signal purely in > user-space? But how would debuggers trap it? I'm not sure how it would > work without a system call. I think the C99 standard allows you to not implement SIGFPE at all but instead rely on applications doing fetestexcept() etc. > >> diff --git a/arch/tile/kernel/sys.c b/arch/tile/kernel/sys.c > >> [...] > >> +ssize_t sys32_readahead(int fd, u32 offset_lo, u32 offset_hi, u32 count) > >> +{ > >> + return sys_readahead(fd, ((loff_t)offset_hi << 32) | offset_lo, count); > >> +} > >> + > >> > >> > > These seem to belong with the other similar functions in compat.c > > > > Except they're also used by the 32-bit platform where there is no compat > mode (the compat code DOES use them too, it's true). I see. AFAICT, all other architectures don't need the wrapper in the 32 bit native case because they define the syscall calling conventions in libc such that they match what the kernel expects for a 64 bit argument (typically split in two subsequent argument slots). Would that work for you as well? > > Just use the sys_mmap_pgoff system call directly, rather than > > defining your own wrappers. Since that syscall is newer than > > asm-generic/unistd.h, that file might need some changes, > > together with fixes to arch/score to make sure we don't break > > its ABI. > > > > It should be OK. Their sys_mmap2() just tail-calls sys_mmap_pgoff() > anyway, so it should be possible to switch mmap2 in asm-generic/unistd.h > to be mmap_pgoff instead. We'll need some user-space changes (our mmap2 > is defined in 4KB units) but that's not hard. Hmm, I forgot about the page size. Actually the definition of sys_mmap2 is to use 4KB units on all architectures except ia64, independent of the real page size. Maybe it's better to keep using sys_mmap/sys_mmap2 after all but then use only one of the two (sys_mmap on 64 bit, sys_mmap2 on 32 bit and compat). Either way should work though. > > It seems that this file fits in the same category as the > > backtrace code. Maybe move both away from arch/tile/kernel into a > > different directory? > > > > I'll think about it. These are both basically disassembly-related, so > maybe an arch/tile/disasm directory with the tile_desc stuff and the > backtracer? I'm not sure it's really worth moving out of > arch/tile/kernel though. Ok. If you leave them in the directory, just split them out into a separate patch on your next submission then. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html