2.6 kernel change in nopage
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 9 lines · Jon Smirl
The headers for nopageXX calls just changed.
struct page * (*nopage)(struct
vm_area_struct* area, unsigned long address, int unused);
changed to:
struct page * (*nopage)(structvm_area_struct* area, unsigned long address, int *type);The DRM drivers need to be adjusted. This probably impacts the BSD builds.
No, this is Linux specific.
How does this patch look?
Signature · 2 lines
Earthling Michel Dänzer | Debian (powerpc), X and DRI developer Software libre enthusiast | http://svcs.affero.net/rm.php?r=daenzer
Index: drmP.h
===================================================================
RCS file: /cvs/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drmP.h,v retrieving revision 1.85
drmP.h +4 -12
@@ -806,18 +819,10 @@ extern int DRM(flush)(struct file *extern int DRM (fasync )(int fd,struct file *filp,int on);/* Mapping support (drm_vm.h) */ -extern struct page *DRM (vm_nopage )(struct vm_area_struct *vma, -unsigned long address, -int write_access); -extern struct page *DRM (vm_shm_nopage )(struct vm_area_struct *vma, -unsigned long address, -int write_access); -extern struct page *DRM (vm_dma_nopage )(struct vm_area_struct *vma, -unsigned long address, -int write_access); -extern struct page *DRM (vm_sg_nopage )(struct vm_area_struct *vma, -unsigned long address, -int write_access); +extern struct page *DRM (vm_nopage )(DRM_NOPAGE_ARGS ); +extern struct page *DRM (vm_shm_nopage )(DRM_NOPAGE_ARGS ); +extern struct page *DRM (vm_dma_nopage )(DRM_NOPAGE_ARGS ); +extern struct page *DRM (vm_sg_nopage )(DRM_NOPAGE_ARGS );extern void DRM (vm_open )(struct vm_area_struct *vma);extern void DRM (vm_close )(struct vm_area_struct *vma);extern void DRM (vm_shm_close )(struct vm_area_struct *vma);
Index: drm_os_linux.h
===================================================================
RCS file: /cvs/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_os_linux.h,v retrieving revision 1.19
drm_os_linux.h +7 -0
@@ -52,6 +52,13 @@ typedef void irqreturn_t;#define DRM_AGP_KERN struct agp_kern_info#endif +/** Page fault handler arguments */ +#if LINUX_VERSION_CODE >KERNEL_VERSION (2 ,6 ,0 ) +#define DRM_NOPAGE_ARGS struct vm_area_struct *vma, unsigned long address, int *type +#else +#define DRM_NOPAGE_ARGS struct vm_area_struct *vma, unsigned long address, int unused +#endif +/** Task queue handler arguments */ #define DRM_TASKQUEUE_ARGS void *arg
Index: drm_vm.h
===================================================================
RCS file: /cvs/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_vm.h,v retrieving revision 1.26
drm_vm.h +28 -16
110 lines not on this page · raw message
Quoted 1 line · Michel Dänzer
How does this patch look?
ugly.
I find using #defines for function arguments ugly beyond belief and makes it really hard to look through code. I 10x rather have an ifdef in the function prototype (which then for the mainstream kernel drm can be removed for non-matching versions) than such obfuscation.
Quoted 10 lines · Arjan van de Ven
Quoted 1 line · Michel Dänzer
How does this patch look?
ugly.
I find using #defines for function arguments ugly beyond belief and makes it really hard to look through code. I 10x rather have an ifdef in the function prototype (which then for the mainstream kernel drm can be removed for non-matching versions) than such obfuscation.
That doesn't strike me as particularly beautiful either... is it really easier for merges, considering that the ugly way is kinda needed for functions which take different arguments on BSD anyway?
Signature · 2 lines
Earthling Michel Dänzer | Debian (powerpc), X and DRI developer Software libre enthusiast | http://svcs.affero.net/rm.php?r=daenzer
Quoted 10 lines · Michel Dänzer
Quoted 8 lines
Quoted 1 line
How does this patch look?
ugly.
I find using #defines for function arguments ugly beyond belief and makes it really hard to look through code. I 10x rather have an ifdef in the function prototype (which then for the mainstream kernel drm can be removed for non-matching versions) than such obfuscation.
That doesn't strike me as particularly beautiful either...
well the advantage is that the ifdefs can just go away in kernel trees of specific versions... (eg unifdef it)
Quoted 2 lines
is it really easier for merges, considering that the ugly way is kinda needed for functions which take different arguments on BSD anyway?
I disagree there. The "BSD takes different arguments" thing *should* be fixed imo by making the common core of the function an inline function, and have one or two (depends if the common core happens to have its arguments in common with one of the oses) OS specific wrappers with the right prototype. This way the difference in error return sign can also be solved in the wrapper instead of with a nasty macro...
The compiler generates the same code, but it's a lot easier to read/review.
Greetings,
Arjan van de Ven
Quoted 11 lines · Arjan van de Ven
Quoted 7 lines · Michel Dänzer
Quoted 5 lines
I find using #defines for function arguments ugly beyond belief and makes it really hard to look through code. I 10x rather have an ifdef in the function prototype (which then for the mainstream kernel drm can be removed for non-matching versions) than such obfuscation.
That doesn't strike me as particularly beautiful either...
well the advantage is that the ifdefs can just go away in kernel trees of specific versions... (eg unifdef it)
Does this look better? Maybe a macro (or a typedef?) for the type of the last argument would still be a good idea? Or is there yet a better way?
Quoted 11 lines
Quoted 2 lines
is it really easier for merges, considering that the ugly way is kinda needed for functions which take different arguments on BSD anyway?
I disagree there. The "BSD takes different arguments" thing *should* be fixed imo by making the common core of the function an inline function, and have one or two (depends if the common core happens to have its arguments in common with one of the oses) OS specific wrappers with the right prototype. This way the difference in error return sign can also be solved in the wrapper instead of with a nasty macro...
The compiler generates the same code, but it's a lot easier to read/review.
Interesting, sounds like food for thought for Eric Anholt. :)
Signature · 2 lines
Earthling Michel Dänzer | Debian (powerpc), X and DRI developer Software libre enthusiast | http://svcs.affero.net/rm.php?r=daenzer
Index: drmP.h
===================================================================
RCS file: /cvs/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drmP.h,v retrieving revision 1.85
drmP.h +28 -12
@@ -806,18 +819,34 @@ extern int DRM(flush)(struct file *extern int DRM (fasync )(int fd,struct file *filp,int on);/* Mapping support (drm_vm.h) */ -extern struct page *DRM (vm_nopage )(struct vm_area_struct *vma, -unsigned long address, -int write_access); -extern struct page *DRM (vm_shm_nopage )(struct vm_area_struct *vma, -unsigned long address, -int write_access); -extern struct page *DRM (vm_dma_nopage )(struct vm_area_struct *vma, -unsigned long address, -int write_access); -extern struct page *DRM (vm_sg_nopage )(struct vm_area_struct *vma, -unsigned long address, -int write_access); +extern struct page *DRM (vm_nopage )(struct vm_area_struct *vma, +unsigned long address, +int +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,0) + * +#endif + type); +extern struct page *DRM (vm_shm_nopage )(struct vm_area_struct *vma, +unsigned long address, +int +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,0) + * +#endif + type); +extern struct page *DRM (vm_dma_nopage )(struct vm_area_struct *vma, +unsigned long address, +int +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,0) + * +#endif + type); +extern struct page *DRM (vm_sg_nopage )(struct vm_area_struct *vma, +unsigned long address, +int +#if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,0) + * +#endif + type);extern void DRM (vm_open )(struct vm_area_struct *vma);extern void DRM (vm_close )(struct vm_area_struct *vma);extern void DRM (vm_shm_close )(struct vm_area_struct *vma);
Index: drm_vm.h
===================================================================
RCS file: /cvs/dri/xc/xc/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel/drm_vm.h,v retrieving revision 1.26
drm_vm.h +44 -8
124 lines not on this page · raw message
Quoted 2 lines · Michel D?nzer
Does this look better? Maybe a macro (or a typedef?) for the type of the last argument would still be a good idea? Or is there yet a better way?
I'm going to regret suggesting this, but how about:
(a) a typedef for the arg itself
(b) a macro and/or inline for the type update
both simultaneously?
So we'd have centralized in one place:
#if /* kernel version > 2.6.0 */
typdef int *third_arg_t;
#define third_arg_update(type) do { *(type) = VM_FAULT_MINOR; } while (0)
#else
typdef int third_arg_t;
#define third_arg_update(type) do { } while (0)
#endif
... and the natural usage that follows.
-- wli
Quoted 6 lines · Michel Dänzer
Quoted 3 lines
well the advantage is that the ifdefs can just go away in kernel trees of specific versions... (eg unifdef it)
Does this look better? Maybe a macro (or a typedef?) for the type of the last argument would still be a good idea? Or is there yet a better way?
My preference is either:
- we can still undo the "nopage" argument change. It's been in the -mm tree for a long time, and it _does_ solve a problem (page fault type accounting), but the problem it solves is potentially so small that we might decide it's ok for 2.6.x.
However, Andrew is king, and besides, it does fix a tiny bug, so I don't think this is what we should do. I just wanted to put it on the table as a possibility.
- Use separate (and trivial) wrapper functions for this. Keep the "real" function the same across everything, and just have a _static_ function (ie no header file declaration crap) that does
linux-new-vm.h:
static int DRM(nopage_interface)(struct vm_area_struct * area,
unsigned long address,
int *type)
{
*type = VM_FAULT_MINOR;
DRM(nopage)(area, address);
} linux-old-vm.h:
static int DRM(nopage_interface)(struct vm_area_struct * area,
unsigned long address,
int unused)
{
DRM(nopage)(area, address);
} drm_vm.h:
/* * Or, poreferably, we could create a symlink and avoid * the #if's at compile-time _entirely_. */ #if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,0) #include "linux-new-vm.h" #else #include "linux-old-v.h #endif .. .nopage = nopage_interface; ..
Done right, the virtualization could be a bit higher still, and maybe the BSD code can do the same thing.
In general, at least I personally _much_ prefer the #ifdef's etc to be outside the code. So even if you don't like to have separate small files for different architectures/ports/versions, I'd at least personally much rather have
#if xxxx int onewholefunction(..) ... #else int onewholefunction(..) ... #endif
rather than the messy
int onewholefunction(
#if xxx
..
#else
..
#endif
(
{
...
#if xxxx
..
#endifThe latter is a huge pain not just to read (you go blind after a while), but it's also painful as _hell_ to merge anywhere else.
In contrast, full-file interfaces for different kernel versions are a _lot_ easier to merge and keep track of. They may look like "duplication", but the advantages are legion. You don't mix different OS's and different versions together, and that makes it much easier to support them all without going crazy.
Linus
Quoted 5 lines · Linus Torvalds
In contrast, full-file interfaces for different kernel versions are a _lot_ easier to merge and keep track of. They may look like "duplication", but the advantages are legion. You don't mix different OS's and different versions together, and that makes it much easier to support them all without going crazy.
Of course there are also advantages to _not_ using the file-per-kernel version scheme. Keeping one set of files means time is not wasted applying the same change to multiple variations, removes the possibility of patches getting applied to one version and not another and simplifies the process of continuing to support old kernel versions. For merging, a bit of test processing on the files could always be used to remove the ugliness and clean things up.
Regards,
Nigel
Signature · 2 lines
My work on Software Suspend is graciously brought to you by LinuxFund.org.
Quoted 3 lines · Nigel Cunningham
Of course there are also advantages to _not_ using the file-per-kernel version scheme.
No there isn't.
The thing is, you should keep those "file-per-OS" files as small as possible, and only contain the things that are literally different. Because:
Quoted 2 lines
Keeping one set of files means time is not wasted
applying the same change to multiple variations
If the files only contain the actual differences, this just isn't an issue. Those files are per-OS _anyway_, so regardless of how you do it (with #ifdef's inside our outside the code etc), you'd have several versions.
And having separate files means that you don't uglify the code for another OS or another version and hide the _real_ issues.
But yes, it assumes that you can cleanly abstract out the differences.
Linus
First of all, thanks for all the suggestions I've received in this thread.
New patch up at http://penguinppc.org/~daenzer/DRI/drm-nopage.diff; does this look acceptable to those who are going to do merges between the trees? :)
Signature · 2 lines
Earthling Michel Dänzer | Debian (powerpc), X and DRI developer Software libre enthusiast | http://svcs.affero.net/rm.php?r=daenzer
Quoted 6 lines · Michel Dänzer
First of all, thanks for all the suggestions I've received in this thread.
New patch up at http://penguinppc.org/~daenzer/DRI/drm-nopage.diff; does this look acceptable to those who are going to do merges between the trees? :)
I like this one a whole lot better than the previous ones... One could argue that you want the do_ function set the pagefault type itself (and just igore the result in the 2.4 variants) but that's minor nitpicking at most.
Quoted 4 lines · Arjan van de Ven
Quoted 3 lines · Michel Dänzer
First of all, thanks for all the suggestions I've received in this thread.
Quoted 8 lines
Quoted 3 lines
New patch up at http://penguinppc.org/~daenzer/DRI/drm-nopage.diff; does this look acceptable to those who are going to do merges between the trees? :)
I like this one a whole lot better than the previous ones... One could argue that you want the
do_function set the pagefault type itself (and just igore the result in the 2.4 variants) but that's minor nitpicking at most.
Okay, thanks, I just committed this.
Signature · 2 lines
Earthling Michel Dänzer | Debian (powerpc), X and DRI developer Software libre enthusiast | http://svcs.affero.net/rm.php?r=daenzer
Quoted 2 lines · Michel D?nzer
No, this is Linux specific.
How does this patch look?
Okay, you did something weird with nopage args, but I thought I did the equivalent of this in the original patch?
-- wli
Quoted 6 lines · William Lee Irwin III
Quoted 2 lines · Michel D?nzer
No, this is Linux specific.
How does this patch look?Okay, you did something weird with nopage args, but I thought I did the equivalent of this in the original patch?
This is about the canonical DRM code in the DRI tree.
Signature · 2 lines
Earthling Michel Dänzer | Debian (powerpc), X and DRI developer Software libre enthusiast | http://svcs.affero.net/rm.php?r=daenzer
Quoted 2 lines · William Lee Irwin III
Quoted 2 lines
Okay, you did something weird with nopage args, but I thought I did the equivalent of this in the original patch?
Quoted 1 line · Michel D?nzer
This is about the canonical DRM code in the DRI tree.
I'm sorry, I'm not going to drive myself insane trying to deal with yet another overweight and insanely ugly BSD emulation layer. I'll stick with canonical kernel.org sources, thank you very much.
-- wli
Quoted 4 lines · Michel Dänzer
Quoted 2 lines
Okay, you did something weird with nopage args, but I thought I did the equivalent of this in the original patch?
This is about the canonical DRM code in the DRI tree.
99.9% of people run the DRM code in the kernel tree, so definitions of canonical might vary. I don't personally see a problem with the ifdefs. The DRI devel tree has to work with anything, the kernel gets the luxury of being able to strip out the defines that aren't needed for that specific release.
Alan