Re: CVS commit: src/sbin/amrctl
What's wrong with printf("%s", NULL)? It produces '(null)', and at least it's visible something is missing there. I think gcc 9.3 is overly eager for this. Is it correct to just omit the parameter altogether and change output format? Jaromir Le dim. 6 sept. 2020 à 04:41, matthew green a écrit : > > Module Name:src > Committed By: mrg > Date: Sun Sep 6 02:34:03 UTC 2020 > > Modified Files: > src/sbin/amrctl: amrctl.c > > Log Message: > avoid calling printf() %s with NULL. > > > To generate a diff of this commit: > cvs rdiff -u -r1.11 -r1.12 src/sbin/amrctl/amrctl.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/sys/kern
Le dim. 2 août 2020 à 15:57, Taylor R Campbell a écrit : > Why does it improve readability? Certainly using cringe language features does not help readability. > What else does -Wvla choke on in src/sys? Some drm drivers, and uvm, particularly uvm_bio.c. I'd like to work towards enabling -Wvla in kernel (i.e. remove all VLAs, same as Linux kernel did in 2018), but I have some other stuff I want to finish before I pick a new project. Jaromir
Re: CVS commit: src/sys/kern
Readability first and foremost in this case. I was exploring if I can disable VLAs for the kernel altogether, this can't be done for now. Nevertheless, this change looked like it would be useful to make anyway. Le dim. 2 août 2020 à 01:15, Taylor R Campbell a écrit : > > > Module Name:src > > Committed By: jdolecek > > Date: Sat Aug 1 11:18:26 UTC 2020 > > > > Modified Files: > > src/sys/kern: subr_autoconf.c > > > > Log Message: > > avoid VLA for the sizeof() calculations > > Why?
Re: CVS commit: src/sys/dev/pci
One of the things which need to be done is calling the if_ioctl always with the IFNET_LOCK() held. Right now it sometimes is, and other times it is not, so it's not possible to rely on it and KASSERT(). As for bnx(4) I did just some basic fixes around making it work with MSI(-X), since I don't really have easy access to the hw for testing. This might change soon, then I might look into making it NET_MPSAFE, after some other bug fixes. Jaromir Le sam. 18 juil. 2020 à 00:54, Jason Thorpe a écrit : > > > > > On Jul 17, 2020, at 3:50 PM, matthew green wrote: > > > > any chance you can look at NET_MPSAFE here etc? :) > > I have a bunch of local changes for this in one of my trees, and I hope to > get back to it after netbsd-10 branches. > > -- thorpej >
Re: CVS commit: src/sys/arch/powerpc/include
Perhaps we can use a similar technique for mips too? There also the kernel actually always uses a compile-time fixed page size AFAICS. Jaromir Le sam. 27 juin 2020 à 04:51, Rin Okuyama a écrit : > > Module Name:src > Committed By: rin > Date: Sat Jun 27 02:51:23 UTC 2020 > > Modified Files: > src/sys/arch/powerpc/include: vmparam.h > > Log Message: > Restrict {MIN,MAX}_PAGE_SIZE for MODULAR || _MODULE, which makes > non-MODULAR kernel a little bit efficient. > > They are also exposed to userland for jemalloc. > > > To generate a diff of this commit: > cvs rdiff -u -r1.22 -r1.23 src/sys/arch/powerpc/include/vmparam.h > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/sys/external/bsd/drm2/dist/drm/nouveau
The static variable was in #ifdef __NetBSD__ part, so I assumed it doesn't influence the merge. Christos already reverted the fallthrough warning fix. Jaromir Le jeu. 13 févr. 2020 à 09:18, matthew green a écrit : > > "Jaromir Dolecek" writes: > > Module Name: src > > Committed By: jdolecek > > Date: Wed Feb 12 20:31:46 UTC 2020 > > > > Modified Files: > > src/sys/external/bsd/drm2/dist/drm/nouveau: nouveau_fbcon.c > > > > Log Message: > > remove superfluous static variable used only to zero attach args > > is this necessary for 3rd party code? > > please try to avoid changing these for cosmetic reasons. > > same for warned code that is forced to warning not error. > the reason we/i didn't change them is to avoid making > future drm updates harder -- they're already *really* hard. > > thanks. > > > .mrg.
Re: CVS commit: src/sys/uvm
Good point, added asserts in genfs_io.c for this. Le dim. 9 déc. 2018 à 23:56, matthew green a écrit : > > "Jaromir Dolecek" writes: > > Module Name: src > > Committed By: jdolecek > > Date: Sun Dec 9 20:45:37 UTC 2018 > > > > Modified Files: > > src/sys/uvm: uvm_bio.c > > > > Log Message: > > for direct map case, avoid PGO_NOBLOCKALLOC when writing, it makes > > genfs_getpages() return unallocated pages using the zero page and > > PG_RDONLY; the old code relied on fault logic to get it allocated, which > > the direct case can't rely on > > > > instead just allocate the blocks right away; pass PGO_JOURNALLOCKED > > so that code wouldn't try to take wapbl lock, this code path is called > > with it already held > > can you assert this if not already done? that the locking is OK, > i mean... > > thanks. > > > .mrg.
Re: CVS commit: src/sys/arch/x86
Maybe I missed something earlier - does KASLR being enabled by default mean that x86 now doesn't any more use the direct map to copy memory pages? Jaromir Le ven. 7 déc. 2018 à 16:47, Maxime Villard a écrit : > > Module Name:src > Committed By: maxv > Date: Fri Dec 7 15:47:11 UTC 2018 > > Modified Files: > src/sys/arch/x86/conf: files.x86 > src/sys/arch/x86/x86: pmap.c > > Log Message: > Add an option to have a static kernel memory layout. This option is > disabled by default - that is to say, KASLR remains enabled by default. > > > To generate a diff of this commit: > cvs rdiff -u -r1.103 -r1.104 src/sys/arch/x86/conf/files.x86 > cvs rdiff -u -r1.312 -r1.313 src/sys/arch/x86/x86/pmap.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/sys/arch/x86/x86
Shouldn't this: memtop |= (uint16_t)mpbios_page[0x414] << 8; be actually << 16 to keep the same semantics? Jaromir Le dim. 8 juil. 2018 à 01:05, Kamil Rytarowski a écrit : > > Module Name:src > Committed By: kamil > Date: Sat Jul 7 23:05:50 UTC 2018 > > Modified Files: > src/sys/arch/x86/x86: mpbios.c > > Log Message: > Remove unaligned access to mpbios_page[] > > Replace unaligned pointer dereference with a more portable construct that > is free from Undefined Behavior semantics. > > sys/arch/x86/x86/mpbios.c:308:11, load of misaligned address > 0x800031c7a413 for type 'const __uint16_t' which requires 2 byte alignment > > Detected with Kernel Undefined Behavior Sanitizer > > > To generate a diff of this commit: > cvs rdiff -u -r1.66 -r1.67 src/sys/arch/x86/x86/mpbios.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. >
Re: CVS commit: src/sys/arch
2018-06-20 18:03 GMT+02:00 Maxime Villard : > I haven't tested but looking at the code it seems to me that this part of > your > change breaks DAZ on native, and I don't see how it can help xsave on xen by > the way Thanks, I'll change the conditional to avoid any potential change of behaviour for native. Yes, I know that fxsave is different than xsave, but on Xen it seems to be somehow connected. The conditional on XSAVE wad been necessary on Xen according to my testing - with no-xsave that function faulted, with xsave active it didn't. The only difference in CPUID for working and faulting code was the OSXSAVE bit. Maybe Xen somehow traps/disables fxsave also with no-xsave, or maybe requires bigger alignment for some reason. I'll check if I can find something to make the fxsave case work for Xen. Jaromir
Re: CVS commit: src/sys/arch
The function is ever called only with x86_fpu_save >= FPU_SAVE_FXSAVE, so there should be no change in functionality AFAICS. 2018-06-20 10:34 GMT+02:00 Maxime Villard : > Le 19/06/2018 à 21:50, Jaromir Dolecek a écrit : >> >> Module Name:src >> Committed By: jdolecek >> Date: Tue Jun 19 19:50:19 UTC 2018 >> >> Modified Files: >> src/sys/arch/x86/include: fpu.h >> src/sys/arch/x86/x86: cpu.c fpu.c identcpu.c >> src/sys/arch/xen/x86: cpu.c >> >> Log Message: >> fix FPU initialization on Xen to allow e.g. AVX when supported by >> hardware; >> only use XSAVE when the the CPUID OSXSAVE bit is set, as this seems to be >> reliable indication >> >> tested with Xen 4.2.6 DOM0/DOMU on Intel CPU, without and with no-xsave >> flag, >> so should work also on those AMD CPUs, which have XSAVE disabled by >> default; >> also tested with Xen DOM0 4.8.3 >> >> fixes PR kern/50332 by Torbjorn Granlund; sorry it took three years to >> address >> >> XXX pullup netbsd-8 > > > I don't understand, and it looks wrong. > > -- fxsave > > fpuinit_mxcsr_mask calls fxsave, not xsave. The availability of fxsave is > controlled by CR4_OSFXSR, not CR4_OSXSAVE. It seems to me that the check you > added in fpuinit_mxcsr_mask is wrong, because it is confusing fxsave and > xsave. > > [x86_fpu_save = FPU_SAVE_FXSAVE] guarantees fxsave is supported. > > fpuinit_mxcsr_mask is called only when this condition is true, so there is > no > need for a check in the first place. > > The reason I disabled the code on Xen, if I remember correctly, was because > of a stack alignment problem. For some reason this variable > > union savefpu fpusave __aligned(16); > > was not always 16-byte aligned on Xen. So fxsave would fault. > > -- xsave > > Please keep the initialization logic consistent. > > xen/x86/cpu.c > 554 if (cpu_feature[1] & CPUID2_OSXSAVE) > 555 cr4 |= CR4_OSXSAVE; > 556 else { > 557 x86_xsave_features = 0; > 558 x86_fpu_save = FPU_SAVE_FXSAVE; > 559 } > > If the availability of xsave on xen is determined by CPUID2_OSXSAVE (as > opposed to CPUID2_XSAVE on native), then put the code in identcpu.c just > like native. > > That is to say > > x86/x86/identcpu.c > 804 /* See if xsave (for AVX) is supported */ > 805 if ((ci->ci_feat_val[1] & CPUID2_XSAVE) == 0) > 806 return; > XXX > XXX #ifdef XEN > XXX /* On Xen we also need to check CPUID2_OSXSAVE */ > XXX if ((ci->ci_feat_val[1] & CPUID2_OSXSAVE) == 0) > XXX return; > XXX #endif > > Note also that your change seems wrong on i386 too, because you enable > fxsave > if we don't have xsave, while we may have neither.
Re: CVS commit: src/sys
2018-05-22 12:18 GMT+02:00 Martin Husemann: > Here are timing results: While 37% speedup is nice, I see there isn't nearly as huge difference as for my machine. Could you please try with some file which fits whole in the page cache? > Unfortunatley with ubc_direct enabled, it panics quickly: There is a comment in uvm_bio.c about some platforms not being able to write to individual bytes atomically, and hence software having to do read/modify/write. Could this be somehow causing the machine check on Alpha? Jaromir
Re: CVS commit: src/sys
The very basic test is to do some moral equivalent of "dd if=foo of=/dev/null bs=64k" with some reasonably sized foo file, with default setting twice (once to test reading off the disk, second off cache), then set ubc_direct to 1 via DDB, remount the filesystem and re-run. Also would be good to check that the transferred contents are correct, e.g. via sha1. If that doesn't trigger any panic, further test would be to run some non-trivial build with ubc_direct set to 1. Jaromir 2018-05-21 9:11 GMT+02:00 Martin Husemann: > On Sun, May 20, 2018 at 08:40:23AM -0700, Jason Thorpe wrote: >> FWIW, I checked in the Alpha changes. They were trivial. SH3 and >> MIPS I didn?t do because there?s extra work to do to handle virtually >> indexed caches. (In the case of SH3, it?s actually the SH4 that has >> the issue, and I do have a Dreamcast, so I may get to that one >> eventually...) > > I can test alpha - is there something specical required for testing? > > Martin
Re: regnsub, regasub
+1 to removal 2018-02-26 18:51 GMT+01:00: > > Any chance we can remove this from libc before releasing 8.0? > it has one user, and the implementation is very specific for a certain > use-case. > > having a DIY one use case function in libc is actually harming the > ability to upstream this, aside from the implementation choices.
Re: CVS commit: src/sys/dev/acpi
Can we have acpi_intr_establish() accept the xname? It's very useful to have the driver name registered for "intrctl list". Jaromir 2017-12-10 17:51 GMT+01:00 Manuel Bouyer: > Module Name:src > Committed By: bouyer > Date: Sun Dec 10 16:51:30 UTC 2017 > > Modified Files: > src/sys/dev/acpi: acpi_util.c files.acpi > Added Files: > src/sys/dev/acpi: acpi_i2c.c acpi_i2c.h acpi_intr.h > > Log Message: > Implement a ACPI helper to fill the property array expected from our I2C > framework from the ACPI tables. > Also implement acpi_intr_establish(), acpi_intr_disestablish() and > acpi_intr_string(). > Needed for the upcoming HID over I2C support, proposed on tech-kern@ > on Dec, 1. > > > To generate a diff of this commit: > cvs rdiff -u -r0 -r1.1 src/sys/dev/acpi/acpi_i2c.c \ > src/sys/dev/acpi/acpi_i2c.h src/sys/dev/acpi/acpi_intr.h > cvs rdiff -u -r1.8 -r1.9 src/sys/dev/acpi/acpi_util.c > cvs rdiff -u -r1.99 -r1.100 src/sys/dev/acpi/files.acpi > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > >
Re: CVS commit: src/games/fortune/datfiles
I'd ask you to revert this nonsense. Jaromir 2017-11-18 21:48 GMT+01:00 Maya Rashish: > Module Name:src > Committed By: maya > Date: Sat Nov 18 20:48:50 UTC 2017 > > Modified Files: > src/games/fortune/datfiles: fortunes fortunes2 fortunes2-o.real > > Log Message: > Remove a few offensive quotes, put in as many new quotes. > > PR bin/52735 > > > To generate a diff of this commit: > cvs rdiff -u -r1.65 -r1.66 src/games/fortune/datfiles/fortunes > cvs rdiff -u -r1.59 -r1.60 src/games/fortune/datfiles/fortunes2 > cvs rdiff -u -r1.15 -r1.16 src/games/fortune/datfiles/fortunes2-o.real > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > >
Re: CVS commit: [jdolecek-ncq] src/sys/dev/ata
2017-06-17 20:00 GMT+02:00 Jonathan A. Kollasch: > > I really think we need to make ata_xfer_init() private to > ata_queue_alloc(), and use ata_get_xfer() everywhere. That we can > fabricate an xfer on the stack anywhere that will trample all over an > active command slot is fundamentally wrong. Any non-NCQ command which goes via atastart() is safe, since that routine makes sure there is only one non-NCQ command being executed by HBA. The on-stack xfer was carried over from previous design. I kept it mostly since I didn't want to change those codepaths to deal with xfer not being available. As far as I see, ahcisata(4) reset should be actually fine, since it does the reset without using the xfers/slots. I had a look at siisata_reset_drive(). That thing indeed can't reliably work as-is. Arguably it should be changed to use the on-stack xfer, currently it e.g. doesn't do anything if all slots are taken, which is wrong. I'll have a look. > ata_queue_downsize() currently appears to leak xfers. Additionally once > you put a QD1 drive on the channel you're stuck there forever, even if > you replace the channel population entirely with QD32 drives. Okay, I need to look at this - original assumption was that it's never called while there are other transfers. This is false with PMP. Jaromir
Re: CVS commit: src/sys/arch/x86/x86
You rock! Thank you. 2017-05-31 2:19 GMT+02:00 Maya Rashish: > Module Name:src > Committed By: maya > Date: Wed May 31 00:19:17 UTC 2017 > > Modified Files: > src/sys/arch/x86/x86: cpu.c > > Log Message: > Do not pause many times between testing if the CPU can go. > > This only impacts QEMU as QEMU's implementation of pause is > significantly slower than its implementation of nop. > > PR kern/51623: running qemu-x86_64 with -smp 4 - the additional > CPUs don't start. > > > To generate a diff of this commit: > cvs rdiff -u -r1.125 -r1.126 src/sys/arch/x86/x86/cpu.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > >
Re: CVS commit: src/sys
2016-11-11 11:52 GMT+01:00 J. Hannken-Illjes: > Why do we need a TAILQ here, the number of deletions after the first > element is nearly zero and the list is quite short. The element would be somewhere towards the end of the list, but likely not the very last one. Yes, the list is usually short (IIRC the limit is around 60 per 1MB of log / 1GB of filesystem), so it might not matter much if we have the error path O(n). > +wapbl_deallocation_free(struct wapbl *wl, struct wapbl_dealloc *wd, > + bool locked) > > Better to let the caller always take/release the lock. I wanted to let the pool_put() be called without the lock, but this might be not very useful optimisation. > Returning a pointer to an arbitrary list element and using it > later is bad design. Would be better to define as: > > wapbl_unregister_deallocation(struct wapbl *wl, daddr_t blk, int len) > { I simply want to have it O(1). I think it is useful to keep the error path there fast, as it would be quite common for wapbl case. Also just passing the (opaque) pointer makes it simpler. Thanks for the BAP_ASSIGN() fix. I started investigating only to later notice you already committed fix. What you think about removing the NULL initialization, to let compiler trigger warning there? Seems modern gcc is intelligent enough to correctly trigger warning, even with the conditional assignment. Of course it might be not very useful, as hopefully this code won't be touched again for a long time. --- ffs_inode.c 11 Nov 2016 10:50:16 - 1.123 +++ ffs_inode.c 11 Nov 2016 20:47:04 - @@ -583,8 +583,8 @@ ffs_indirtrunc(struct inode *ip, daddr_t int i; struct buf *bp; struct fs *fs = ip->i_fs; - int32_t *bap1 = NULL; - int64_t *bap2 = NULL; + int32_t *bap1; + int64_t *bap2; struct vnode *vp; daddr_t nb, nlbn, last; char *copy = NULL; Jaromir
Re: CVS commit: src/sys
2016-11-07 12:11 GMT+01:00 J. Hannken-Illjes: > It gets used when a block of block pointers has been deallocated and > brelsed, that is NOT written to disk. If we allow the deallocation of > this block to fail and ufs_truncate_retry() runs ffs_truncate again > it will read the block from disk and free already freed blocks. Good point. The same is however true for ALL the calls in the chain, so then we should have to really use FORCE everywhere. One solution is to always bwrite() or bdwrite() back even fully zeroed block for wapbl case instead of brelse(BC_INVAL). I think that for fsck to reliably recover from crash within truncate, this might actually be needed also for !wapbl case. Another way how to solve this would be to try to register the deallocation and on error bail out, before the diving. It would require cancelling the registration if the diving call returns EAGAIN however. > You mean this: > > error = ffs_indirtrunc(ip, nlbn, FFS_FSBTODB(fs, nb), > (daddr_t)-1, level - 1, countp); > - if (error) > - goto out; > + if (error == EAGAIN) > + goto out; > + else if (error && !allerror) > + allerror = error; No, I mean the copy logic and big blocks with condition on ip->i_ump->um_mountp->mnt_wapbl. > I don't understand ... do you want to split into two diffs? I prefer to split commits into incremental changes if reasonably possible, so it's easier to review and revise. That's all. That's why I prefer the fix for immediate corruption to go separately. Jaromir
Re: CVS commit: src/sys
2016-11-07 10:25 GMT+01:00 J. Hannken-Illjes: > The first part should not be necessary. After the loop we should have > "i == last" -- from a quick look "i < last" is impossible. Yes, I know - it's just to make the diff vs 1.117 smaller and hence easier to review. I want to commit this separately. > The second part is right and responsible for most of the panics. Right. > Your patch still doesn't address my second observation, if the > machine crashs inside ffs_truncate we leave the file system in a > state fsck can't handle. The blocks we freed get attached to other > nodes before they get cleared from our on-disk copy leading to > duplicate blocks. That patch is not really very ideal. One is UFS_WAPBL_REGISTER_DEALLOCATION_FORCE(), which should only be used as desperate measure. Second is that the patch adds IMO too much difference for wapbl and !wapbl case, to already quite complicated function. Also it's slightly confusing that the !wapbl change now doesn't support failure in the middle any more - it will just leave the block inconsistent. I need to think a little about this, maybe there is some better way. Anyway, IMO eventual change to allow fsck to DTRT after crash in ffs_truncate() should go separately from the crash fix. Jaromir
Re: CVS commit: src/sys
> On Fri, Nov 04, 2016 at 04:44:10PM +0100, J. Hannken-Illjes wrote: >> - This change results in "panic: ffs_blkfree_common: freeing free block" >> if I put a file system under stress (*1). >> >> - I suppose not zeroing the blocks to be freed before freeing them >> makes the life of fsck harder. >> >> - Running "brelse(bp, BC_INVAL)" doesn't look OK. The brelse(bp, BC_INVAL) call was there before as well, but the condition changed and is wrong. I can repeat the problem with your script and the packaged fsx (thanks Thomas). Whipped up a patch to what looked wrong there, and it no longer panics for me. Patch is attached. I'll test further and commit it tomorrow. Jaromir Index: ffs_inode.c === RCS file: /cvsroot/src/sys/ufs/ffs/ffs_inode.c,v retrieving revision 1.118 diff -u -p -r1.118 ffs_inode.c --- ffs_inode.c 28 Oct 2016 20:38:12 - 1.118 +++ ffs_inode.c 6 Nov 2016 23:09:15 - @@ -675,18 +675,18 @@ ffs_indirtrunc(struct inode *ip, daddr_t * Recursively free blocks on the now last partial indirect block. */ if (level > SINGLE && lastbn >= 0) { - nb = RBAP(ip, last); + last = lastbn % factor; + nb = RBAP(ip, i); if (nb != 0) { error = ffs_indirtrunc(ip, nlbn, FFS_FSBTODB(fs, nb), - lastbn % factor, level - 1, - countp); + last, level - 1, countp); if (error) goto out; } } out: - if (RBAP(ip, 0) == 0) { + if (lastbn < 0 && error == 0) { /* all freed, release without writing back */ brelse(bp, BC_INVAL); } else {
Re: CVS commit: src/sys/arch/x86
I see Robert added hack meanwhile, I'll look at proper fix tomorrow. Jaromir 2016-10-16 0:23 GMT+02:00 Joerg Sonnenberger: > On Sat, Oct 15, 2016 at 04:46:15PM +, Jaromir Dolecek wrote: >> Module Name: src >> Committed By: jdolecek >> Date: Sat Oct 15 16:46:14 UTC 2016 >> >> Modified Files: >> src/sys/arch/x86/acpi: acpi_machdep.c >> src/sys/arch/x86/include: isa_machdep.h >> src/sys/arch/x86/isa: isa_machdep.c >> src/sys/arch/x86/pci: pciide_machdep.c >> >> Log Message: >> provide intr xname > > You broke the Xen kernels on AMD64. Xen's machine/intr.h version is > different. > > Joerg