Re: kobj methods (DEVMETHOD) that have differing signatures (in src/sys)

2009-02-04 Thread M. Warner Losh
In message: <4989e87b.5010...@icyb.net.ua>
Andriy Gapon  writes:
: 
: This based on the (much) earlier proposal described here:
: http://lists.freebsd.org/pipermail/freebsd-arch/2008-April/007982.html
: 
: The patch was applied to the recent current sources and LINT kernels for
: all architectures that have LINT/NOTES (i.e. arm excluded) were built.
: 
: Here's a link to the list of files and line numbers where KOBJ methods
: are set with functions that have differing signatures:
: http://www.icyb.net.ua/~avg/kobj_method_sigs.txt
: 
: List of the most common issues can be found at the first link.
: 
: Hope this is useful.

This is very helpful.  I'll work through the low-hanging fruit.  If
others want to work as well, please ping me.

Warner
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"


NO_WERROR vs kernel builds

2009-02-04 Thread Andriy Gapon

It seems that kernel builds ignore NO_WERROR.
Is this on purpose or by accident?

I think that this happens because of the following lines in
sys/conf/kern.pre.mk:

.if ${CC} != "icc"
CFLAGS+= -fno-common -finline-limit=${INLINE_LIMIT}
CFLAGS+= --param inline-unit-growth=100
CFLAGS+= --param large-function-growth=1000
.if ${MACHINE_ARCH} == "amd64" || ${MACHINE} == "i386" || \
${MACHINE_ARCH} == "ia64" || ${MACHINE_ARCH} == "powerpc" || \
${MACHINE_ARCH} == "sparc64"
WERROR?= -Werror
.endif
.endif

I had to specify WERROR= on make's command line to catch a certain kind
of warnings in bulk instead of one by one. This was not obvious.

-- 
Andriy Gapon
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"


kobj methods (DEVMETHOD) that have differing signatures (in src/sys)

2009-02-04 Thread Andriy Gapon

This based on the (much) earlier proposal described here:
http://lists.freebsd.org/pipermail/freebsd-arch/2008-April/007982.html

The patch was applied to the recent current sources and LINT kernels for
all architectures that have LINT/NOTES (i.e. arm excluded) were built.

Here's a link to the list of files and line numbers where KOBJ methods
are set with functions that have differing signatures:
http://www.icyb.net.ua/~avg/kobj_method_sigs.txt

List of the most common issues can be found at the first link.

Hope this is useful.

-- 
Andriy Gapon
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"


Re: write-only variables in src/sys/ - possible bugs

2009-02-04 Thread Christoph Mallon

Christian Peron schrieb:

On Wed, Feb 04, 2009 at 03:54:41PM +0100, Christoph Mallon wrote:
[..]
Yes, function arguments are considered being read. The problem is 
different here: mtod() should be a macro, but the macro declaration was 
missing (*cough* hacked build process *cough*). So the parser tried to 
parse this as function call. Then it hit the "void *", which confused it 
- it got a type while parsing an expression. I improved the error 
correction, resolved a few other problems, too, and generated a new list:


http://tron.homeunix.org/unread_variables.log
(The list has a date at the top, if it is missing, you see the old list 
in your browser cache)


The false positives, which you mentioned, are gone now - thanks for 
reporting this. The list now contains about 1.000 entries and about 60 
concern variables named 'error'.


Also.. one other thing I noticed:

void
bpf_buffer_append_mbuf(struct bpf_d *d, caddr_t buf, u_int offset, void *src,
u_int len)
{   
const struct mbuf *m;

u_char *dst;
u_int count;

m = (struct mbuf *)src;

dst = (u_char *)buf + offset;
while (len > 0) {
if (m == NULL)
panic("bpf_mcopy");
count = min(m->m_len, len);
bcopy(mtod(m, void *), dst, count);
m = m->m_next;
dst += count;
len -= count;
}
}

  dst += count

In this expression, both dst and count are read since this is the
same thing as:

  dst = dst + count;


No, the analysis *explicitly* marks "x" in neither "x += 1" nor "x = x + 
1" as read. The value of the variable in these expressions is only read 
to calculate its own new value. Therefore it will complain about


int x = 23;
x++;
x += 1;
x = x + 1;
return 0;

This is not a bug, it is a feature. (:
The problem here solely was the insufficient error recovery in the 
bcopy() line, which caused the only "real" user of "dst" to disappear. I 
corrected this problem and as you can see the false positives are no 
longer on the list.

___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"


Re: write-only variables in src/sys/ - possible bugs

2009-02-04 Thread Christian Peron
On Wed, Feb 04, 2009 at 03:54:41PM +0100, Christoph Mallon wrote:
[..]
> 
> Yes, function arguments are considered being read. The problem is 
> different here: mtod() should be a macro, but the macro declaration was 
> missing (*cough* hacked build process *cough*). So the parser tried to 
> parse this as function call. Then it hit the "void *", which confused it 
> - it got a type while parsing an expression. I improved the error 
> correction, resolved a few other problems, too, and generated a new list:
> 
> http://tron.homeunix.org/unread_variables.log
> (The list has a date at the top, if it is missing, you see the old list 
> in your browser cache)
> 
> The false positives, which you mentioned, are gone now - thanks for 
> reporting this. The list now contains about 1.000 entries and about 60 
> concern variables named 'error'.

Also.. one other thing I noticed:

void
bpf_buffer_append_mbuf(struct bpf_d *d, caddr_t buf, u_int offset, void *src,
u_int len)
{   
const struct mbuf *m;
u_char *dst;
u_int count;

m = (struct mbuf *)src;
dst = (u_char *)buf + offset;
while (len > 0) {
if (m == NULL)
panic("bpf_mcopy");
count = min(m->m_len, len);
bcopy(mtod(m, void *), dst, count);
m = m->m_next;
dst += count;
len -= count;
}
}

  dst += count

In this expression, both dst and count are read since this is the
same thing as:

  dst = dst + count;
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"


Re: blockable sleep lock (sleep mutex) 16

2009-02-04 Thread Nikola Knežević

On 2 Feb 2009, at 19:09 , Julian Elischer wrote:

It says "non-sleepable locks", yet it classifies click_instance  
as  sleep mutex. I think witness code should emit messages which  
are more  clear.
It is confusing, but you can't do an M_WAITOK malloc while holding  
a mutex.  Basically, sleeping actually means calling "*sleep()  
(such as mtx_sleep()) or cv_*wait*()".  Blocking on a mutex is not  
sleeping, it's "blocking".  Some locks (such as sx(9)) do "sleep"  
when you contest them.  In the scheduler, sleeping and blocking are  
actually quite different (blocking uses turnstiles that handle  
priority inversions via priority propagation, sleeping uses sleep  
queues which do not do any of that).  The underyling idea is that  
mutexes should be held for "short" periods of time, and that any  
sleeps are potentially unbounded.  Holding a mutex while sleeping  
could result in a mutex being held for a long time.



the locking overview page
man 9 locking
tries to explain this..
I've been pestering John to proofread it and make suggestiosn for a  
while now.



Thanks John and Julian. I agree, man pages should be more clear :)

I've switched from using mtx to sx locks, since they offer sleeping  
while hold.


Unfortunately, I've ran into something really weird now, when I unload  
the module:

---8<---
#0  doadump () at pcpu.h:195
#1  0x8049ef98 in boot (howto=260) at /usr/src/sys/kern/ 
kern_shutdown.c:418

#2  0x8049f429 in panic (fmt=Variable "fmt" is not available.
) at /usr/src/sys/kern/kern_shutdown.c:574
#3  0x8075cd26 in trap_fatal (frame=0xc, eva=Variable "eva" is  
not available.

) at /usr/src/sys/amd64/amd64/trap.c:764
#4  0x8075da62 in trap (frame=0x87699940) at /usr/src/ 
sys/amd64/amd64/trap.c:290
#5  0x80743bfe in calltrap () at /usr/src/sys/amd64/amd64/ 
exception.S:209

#6  0x8052a411 in strcmp (s1=0x80824a0c "sigacts",
s2=0x877cd3a9 )  
at /usr/src/sys/libkern/strcmp.c:45
#7  0x804d7c61 in enroll (description=0x80824a0c  
"sigacts", lock_class=0x80a19fe0)

at /usr/src/sys/kern/subr_witness.c:1439
#8  0x804d7fb1 in witness_init (lock=0xff00016f4ca8) at / 
usr/src/sys/kern/subr_witness.c:618
#9  0x8049fd31 in sigacts_alloc () at /usr/src/sys/kern/ 
kern_sig.c:3280
#10 0x80481121 in fork1 (td=0xff0001384a50, flags=20,  
pages=Variable "pages" is not available.

) at /usr/src/sys/kern/kern_fork.c:453
#11 0x80481450 in fork (td=0xff0001384a50, uap=Variable  
"uap" is not available.

) at /usr/src/sys/kern/kern_fork.c:106
#12 0x8075d260 in syscall (frame=0x87699c80) at /usr/ 
src/sys/amd64/amd64/trap.c:907
#13 0x80743e0b in Xfast_syscall () at /usr/src/sys/amd64/amd64/ 
exception.S:330

#14 0x000800ca0a6c in ?? ()
--->8---

and in fra 7:
(kgdb) p *w
$5 = {w_name = 0x877cd3a9 bounds>, w_class = 0x80a19fe0, w_list = {
stqe_next = 0x80accce0}, w_typelist = {stqe_next =  
0x80accce0}, w_children = 0x0,
  w_file = 0x877d1fa0 bounds>, w_line = 307, w_level = 0, w_refcount = 2,
  w_Giant_squawked = 0 '\0', w_other_squawked = 0 '\0',  
w_same_squawked = 0 '\0', w_displayed = 0 '\0'}

(kgdb) p *w->w_class
$6 = {lc_name = 0x808564e0 "sleep mutex", lc_flags = 9,  
lc_ddb_show = 0x80492e6b ,
  lc_lock = 0x804938be , lc_unlock =  
0x804933fc }


This happens after modevent exists.

What puzzles me here is w_refcount of 2, while w_name is out of  
bounds. Locks I've created I properly destroyed (at least I think I  
did :)).


Cheers,
Nikola
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"


Re: write-only variables in src/sys/ - possible bugs

2009-02-04 Thread Christoph Mallon

Christian Peron schrieb:

I started following up on this and ran into an issue for these:

sys/net/bpf_buffer.c:133: warning: variable 'dst' is never read
sys/net/bpf_buffer.c:134: warning: variable 'count' is never read
sys/net/bpf_buffer.c:142: warning: variable 'dst' is never read


/*
 * Scatter-gather data copy from an mbuf chain to the current kernel buffer.
 */
void
bpf_buffer_append_mbuf(struct bpf_d *d, caddr_t buf, u_int offset, void *src,
u_int len)
{
const struct mbuf *m;
u_char *dst;
u_int count;
 
m = (struct mbuf *)src;

dst = (u_char *)buf + offset;
while (len > 0) {
if (m == NULL)
panic("bpf_mcopy");
count = min(m->m_len, len);
bcopy(mtod(m, void *), dst, count);
m = m->m_next;
[..]

Does it not consider being passed as an argument to a function as
being read?


Yes, function arguments are considered being read. The problem is 
different here: mtod() should be a macro, but the macro declaration was 
missing (*cough* hacked build process *cough*). So the parser tried to 
parse this as function call. Then it hit the "void *", which confused it 
- it got a type while parsing an expression. I improved the error 
correction, resolved a few other problems, too, and generated a new list:


http://tron.homeunix.org/unread_variables.log
(The list has a date at the top, if it is missing, you see the old list 
in your browser cache)


The false positives, which you mentioned, are gone now - thanks for 
reporting this. The list now contains about 1.000 entries and about 60 
concern variables named 'error'.

___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"


Re: bus_dma (9). What exactly means "Loading of memory allocation" ?

2009-02-04 Thread John Baldwin
On Monday 02 February 2009 11:23:33 am Alexej Sokolov wrote:
> Hi,
> thanx for your answer.  I checked the source code of the *dma() functions.
> If I understand it correctly, "loading of memory allocation" means the
> following:
> 
> 1. At first memory allocation should be done: bufp = *alloc(sizeof )
> 2. then in ... _bus_dmamap_load_buffer() we get physical addres of allocated
> buffer:
>  if (pmap)
>  curaddr = pmap_extract(pmap, vaddr);
>  else
>  curaddr = pmap_kextract(vaddr);
> 
> ... then some "magic" with bouncing
> 
> 3. then physical address will passed to dmat->segments
> segs[seg].ds_addr = curaddr;
> segs[seg].ds_len = sgsize;
> 
> Ok, it all means: getting of physical address of allocated buffer. If
> physical space not accessble for device, allocating bounce buffers. Getting
> of physical addresses of allocated buffers. And then put these physical
> addresses and sizes of buffers in  dmat->segments array. <- loading of
> memory allocation (-:

Yes.  On architectures with an IOMMU, the load may also program entries into 
the IOMMU for the specified buffer and then populate the S/G array with the 
associated DMA addresses (sparc64 uses this).  I think the "load" name has 
more to do with this case in that you are "loading" a buffer into the DMA 
virtual address space (with IOMMUs you have a separate virtual address space 
for DMA that is not 1:1 with physical addresses as on i386 machines).  If you 
look at bus_dma as basically implementing an abstract IOMMU on all 
architectures then it might make a bit more sense.

-- 
John Baldwin
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"