Re:TCP transmission error

classic Classic list List threaded Threaded
42 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re:TCP transmission error

boter
Hi all,

    Firstly thanks for Richard's reply.  yes, I am using lwip on the
AT91SAM7X256.
The TCP server is based on the demo lwip_Demo_Rowley_ARM7 from FreeRTOS
v3.2.2,
 I also think  I should free the memory. but how can I do it.  now I list
the important program lines

      while((pxRxBuffer = netconn_recv( pxNetCon )) != NULL)
      {
            netbuf_data(pxRxBuffer, ( void * ) &pcRxString, &usLength);
            netconn_write(pxNetCon, pcRxString, usLength, NETCONN_COPY);
      }
part of program above is loop .
while((pxRxBuffer = netconn_recv( pxNetCon )) != NULL) ,this line will wait
for a new
incoming data , netbuf_data(pxRxBuffer, ( void * ) &pcRxString, &usLength);
this line will
obtain the payload.  netconn_write(pxNetCon, pcRxString, usLength,
NETCONN_COPY);
this line will write back the payload , how can I free the memory . should I
use the netbuf_delete()
to free the memory ?

thanks
Boter
Dec 30, 2005

----- Original Message -----
From: <[hidden email]>
To: <[hidden email]>
Sent: Thursday, December 29, 2005 8:42 AM
Subject: lwip-users Digest, Vol 28, Issue 27


> Send lwip-users mailing list submissions to
> [hidden email]
>
> To subscribe or unsubscribe via the World Wide Web, visit
> http://lists.nongnu.org/mailman/listinfo/lwip-users
> or, via email, send a message with subject or body 'help' to
> [hidden email]
>
> You can reach the person managing the list at
> [hidden email]
>
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of lwip-users digest..."
>
>
> Today's Topics:
>
>   1. Server Program in Raw API without OS
>      (=?big5?q?=B3=AF=20=A4p=BE=D6?=)
>   2. TCP  transmission error (boter)
>   3. Re: TCP  transmission error (FreeRTOS Info)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Thu, 29 Dec 2005 10:38:20 +0800 (CST)
> From: =?big5?q?=B3=AF=20=A4p=BE=D6?= <[hidden email]>
> Subject: [lwip-users] Server Program in Raw API without OS
> To: [hidden email]
> Message-ID: <[hidden email]>
> Content-Type: text/plain; charset="big5"
>
>
> Hi all:
>      I am going to do a project using LwIP without OS . my program can
> transmited data yet . However , my tutor ask me to let the program can
> process multi-connection .  My question is that
>  1.  How can i record that connection ? can i use a array(struct
> tcp_pcb[]) after
>       listen() ?
>  2. If using array can work , How can i handle the closing ?
>
> _______________________________________
> YM - Â÷½u°T®§
> ´Nºâ§A¨S¦³¤Wºô¡A§AªºªB¤Í¤´¥i¥H¯d¤U°T®§µ¹§A¡A·í§A¤Wºô®É´N¯à¥ß§Y¬Ý¨ì¡A¥ô¦ó»¡¸Ü³£ÉN¨«¥¢¡C
> http://messenger.yahoo.com.hk
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:
> http://lists.gnu.org/pipermail/lwip-users/attachments/20051229/b4279475/attachment.html
>
> ------------------------------
>
> Message: 2
> Date: Thu, 29 Dec 2005 18:52:42 -0800
> From: "boter" <[hidden email]>
> Subject: [lwip-users] TCP  transmission error
> To: "lwip-users" <[hidden email]>
> Message-ID: <000801c60cec$1ac3e9e0$9200a8c0@Boter>
> Content-Type: text/plain; charset="gb2312"
>
> Hi all,
>
>    I  writed  a simple TCP-server program base on Rowley webserver demo
> using lwip.
> the server function is that transmit back what it received.  when the
> function netconn_recv()
> is executed the fifth time ,the error will occur . I traced the program .
> I found the case is the
> function netconn_recv() cann't obtain memory for the new incoming data.
> (the return of
> buf = memp_malloc(MEMP_NETBUF) in netconn_recv() function is NULL) . so
> the server
> program cann't receive incoming data again.  I changed MEM_SIZE  from 2000
> to 4000 ,but
> the result is  same. if anyone need the source code ,please email to me .
> or anybody send
> me your successful program of TCP communication. let me appreciate your
> experience.
>    My email address : [hidden email]
>
> Thanks
> Boter
> 29 Dec, 2005
>
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:
> http://lists.gnu.org/pipermail/lwip-users/attachments/20051229/a2b62a83/attachment.html
>
> ------------------------------
>
> Message: 3
> Date: Thu, 29 Dec 2005 12:35:56 -0000
> From: "FreeRTOS Info" <[hidden email]>
> Subject: Re: [lwip-users] TCP  transmission error
> To: "Mailing list for lwIP users" <[hidden email]>
> Message-ID: <00b201c60c74$6c8912a0$[hidden email]>
> Content-Type: text/plain; charset="gb2312"
>
>>    I  writed  a simple TCP-server program base on Rowley webserver demo
> using lwip.
>
> For ARM?
>
>>the server function is that transmit back what it received.  when the
> function netconn_recv()
>>is executed the fifth time ,the error will occur .
>
> Sounds like you are not freeing memory correctly.
>
>>or anybody send
>>me your successful program of TCP communication.
>>let me appreciate your experience.
>
> I don't know if this is what you have seen already - but the FreeRTOS.org
> WEB site (link below) has an lwIP server demo also using Rowley tools.
> May
> help.
>
> Regards,
> Richard.
>
> http://www.FreeRTOS.org
>
>
>
>
>
> ------------------------------
>
> _______________________________________________
> lwip-users mailing list
> [hidden email]
> http://lists.nongnu.org/mailman/listinfo/lwip-users
>
> End of lwip-users Digest, Vol 28, Issue 27
> ******************************************
>




_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Bug in pbuf.c regarding PBUF_POOL

Goldschmidt Simon
Hi all,

I was experiencing problems with pbufs of type PBUF_POOL which did seem
very odd (ref-counts were wrong, payload was set back to start of
buffer...). Now, I used some old version (1.1.0 I think), where those
problems did not occur. After diff'ing those two versions, I found that
pbuf_pool_alloc uses a new locking scheme which, to me, seems totally
buggy!

On allocating a PBUF_POOL, pbuf_pool_alloc() locks the pool by setting a
variable to 1 (not thread-safe!!!), but when freeing a PBUF_POOL,
PBUF_POOL_FREE() still locks the pool the old way using a semaphore. So
there are two locks! They prevent running pbuf_pool_alloc() twice at the
same time or running PBUF_POOL_FREE() twice, but they don't gain
exclusive access to the pool!

Though this is only relevant if SYS_LIGHTWEIGHT_PROT is set to 0, but
that was the case with me...
Setting SYS_LIGHTWEIGHT_PROT made the problem disappear.

Hope for some comments on this...


Simon
 


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Bug in pbuf.c regarding PBUF_POOL

Kieran Mansley
On Fri, 2006-10-06 at 11:47 +0200, Goldschmidt Simon wrote:
> Hi all,
>
> I was experiencing problems with pbufs of type PBUF_POOL which did seem
> very odd (ref-counts were wrong, payload was set back to start of
> buffer...). Now, I used some old version (1.1.0 I think), where those
> problems did not occur. After diff'ing those two versions, I found that
> pbuf_pool_alloc uses a new locking scheme which, to me, seems totally
> buggy!

Can any of the developers comment on this change?  1.1.0 is not that
old, so should be quite recent, and hopefully still relatively fresh in
the mind, but I don't remember anything about this.

Kieran



_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Bug in pbuf.c regarding PBUF_POOL

Kieran Mansley
In reply to this post by Goldschmidt Simon
On Fri, 2006-10-06 at 11:47 +0200, Goldschmidt Simon wrote:
> Hi all,
>
> I was experiencing problems with pbufs of type PBUF_POOL which did seem
> very odd (ref-counts were wrong, payload was set back to start of
> buffer...). Now, I used some old version (1.1.0 I think), where those
> problems did not occur. After diff'ing those two versions, I found that
> pbuf_pool_alloc uses a new locking scheme which, to me, seems totally
> buggy!

Can you check that it was v1.1.0 that worked for you - looking at that
it seems to match the description of the problems you are seeing in
1.1.1

This might be of interest:

http://lists.gnu.org/archive/html/lwip-users/2003-01/msg01899.html

Kieran



_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Bug in pbuf.c regarding PBUF_POOL

Kieran Mansley
In reply to this post by Goldschmidt Simon
On Fri, 2006-10-06 at 11:47 +0200, Goldschmidt Simon wrote:

> Hope for some comments on this...
>

Sorry for the multiple replies, but I knew this had been discussed
recently, I was just searching in the wrong mailing list:

http://lists.gnu.org/archive/html/lwip-devel/2006-03/msg00000.html

(and subsequent replies).

i.e. I think we concluded that none of us understand how it's supposed
to work either.

The post from Adam that I pointed at in my last reply suggests that it's
not designed to make it thread safe - the core of lwIP is not thread
safe and you as the porter must ensure that only one thread uses it at
once - and so it may be doing what was originally intended, but that
this is not what you want it to do.

I still reckon it's not doing much that is useful though, especially as
pbuf_pool_free_lock is never set to anything other than 0, and so it
should be purged.

Kieran



_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Bug in pbuf.c regarding PBUF_POOL

Christiaan Simons
In reply to this post by Goldschmidt Simon

Hi Kieran,

> Can any of the developers comment on this change?  1.1.0 is not that
> old, so should be quite recent, and hopefully still relatively fresh in
> the mind, but I don't remember anything about this.

Being the single currently active developer:

I can't see any changes for the pbuf.c locking code between 1.1.0 and
1.1.1,
maybe these changes are further down history lane.

I do remember being totally puzzled over this locking stuff, but I didn't
dare touching it...

I'll need to follow up the mem_malloc() thing frist (that will be next
week).

Christiaan Simons

Hardware Designer
Axon Digital Design

http://www.axon.tv



_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

RE: Bug in pbuf.c regarding PBUF_POOL

Goldschmidt Simon
In reply to this post by Kieran Mansley
Hi,

1:
Sorry to be so 'unprofessional' :)
But:
The 1.1.0 version I was using was provided by Altera with the NIOS2
devkit. I just compared this version to the oringinal 1.1.0 and saw that
Altera changed that part... They simply just used the semaphore used by
PBUF_POOL_FREE() in pbuf_pool_alloc() as well. I can submit a patch
doing this if anyone cares...

2:
The other question is: Why did nobody get corrupted pbufs like I do?
Have you all set SYS_LIGHTWEIGHT_PROT to 1? (since it's by default set
to 0...)

3:
OK, maybe the stack is not supposed to be multithreaded, BUT: I have a
separate thread for receiveing, and that thread must allocate fresh
pbufs (PBUF_POOL in my case) so where should I do that if not in a
seperate thread? With SYS_LIGHTWEIGHT_PROT=1, INTs are diables (=
threadsafe); in PBUF_POOL_FREE(), a semaphore is used (=threadsafe), so
I still think the design goal is to make pbufs and memory access thread
safe!

4:
The reason pbuf_pool_free_lock is never set to anything except 0 is that
it is not used on the FREE-side. What strikes me more is, that
pbuf_pool_alloc_lock is never tested against, only set to a value. But
that seems only to be a typo (in v1.1.1, pbuf.c line 146 should use
pbuf_pool_alloc_lock instead of pbuf_pool_free_lock).

@Kieran: The discussion you pointed out is 3 years old and the version
of pbuf.c at that time (cvs #1.10 or something) seems not to have
SYS_LIGHTWEIGHT_PROT included. Disabeling interrupts is even faster than
testing and incrementing a variable, so SYS_LIGHTWEIGHT_PROT=0 should
youse the sem, in my opinion...

@Christiann: I read the post on lwip-devel Kieran pointed me to. Seems
you had the same idea like me... Could you change the pbuf_pool_alloc()
stuff to use the semaphore, too? I know you didn't want to touch that
one, but the way it is now it's broken anyway... The speed is poor
anyway if SYS_LIGHTWEIGHT_PROT=0, since PBUF_POOL_FREE uses the
semaphore, too.

Oh, and thanks for the fast replies!

Simon


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

RE: Bug in pbuf.c regarding PBUF_POOL

Goldschmidt Simon
In reply to this post by Kieran Mansley
Hi,

I got still 2 comments on the locking:

1. If acces to pbuf pool was not supposed to be thread-safe, why are
pbuf_pool_free_lock and pbuf_pool_alloc_lock declared volatile?

2. @Christiaan regarding
http://lists.gnu.org/archive/html/lwip-devel/2006-03/msg00000.html:
There is no double-locking since SYS_ARCH_PROTECT macros are defined to
do nothing if (SYS_LIGHTWEIGHT_PROT == 0).

Simon


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Compiler warnings

Goldschmidt Simon
In reply to this post by Kieran Mansley
Hi all,

I was just wandering if anybody has looked into ANSI-compatibility for
the last year... I found a post from 09/2004 about that (and an answer
of Kieran, who seems to be pretty active there). But has anybody ever
cared to do something about it? I get so many warnings when compiling
lwIP that I took the effort changing most of them.
Though in some cases (mainly pointer arithmetic -> alignment), a fast
fix is not possible, most warnings are cast issues which could easily be
fixed.

Anyone else but me cares about that? Hope so...

Simon.

--
Simon Goldschmidt
PA-PG-FT

Pepperl+Fuchs GmbH
Koenigsberger Allee 87
68307 MANNHEIM / GERMANY
Phone:  +49 621 776-1714
Telefax: +49 621 776-1557
E-Mail:[hidden email]
www.pepperl-fuchs.com

PROTECTING YOUR PROCESS

 

> -----Original Message-----
> From:
> lwip-users-bounces+sgoldschmidt=[hidden email]
> g
> [mailto:lwip-users-bounces+sgoldschmidt=de.pepperl-fuchs.com@n
> ongnu.org] On Behalf Of Kieran Mansley
> Sent: Friday, October 06, 2006 12:17 PM
> To: Mailing list for lwIP users
> Subject: Re: [lwip-users] Bug in pbuf.c regarding PBUF_POOL
>
> On Fri, 2006-10-06 at 11:47 +0200, Goldschmidt Simon wrote:
>
> > Hope for some comments on this...
> >
>
> Sorry for the multiple replies, but I knew this had been
> discussed recently, I was just searching in the wrong mailing list:
>
> http://lists.gnu.org/archive/html/lwip-devel/2006-03/msg00000.html
>
> (and subsequent replies).
>
> i.e. I think we concluded that none of us understand how it's
> supposed to work either.
>
> The post from Adam that I pointed at in my last reply
> suggests that it's not designed to make it thread safe - the
> core of lwIP is not thread safe and you as the porter must
> ensure that only one thread uses it at once - and so it may
> be doing what was originally intended, but that this is not
> what you want it to do.
>
> I still reckon it's not doing much that is useful though,
> especially as pbuf_pool_free_lock is never set to anything
> other than 0, and so it should be purged.
>
> Kieran
>
>
>
> _______________________________________________
> lwip-users mailing list
> [hidden email]
> http://lists.nongnu.org/mailman/listinfo/lwip-users
>


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Compiler warnings

Kieran Mansley
On Fri, 2006-10-06 at 13:51 +0200, Goldschmidt Simon wrote:
> Hi all,
>
> I was just wandering if anybody has looked into ANSI-compatibility for
> the last year... I found a post from 09/2004 about that (and an answer
> of Kieran, who seems to be pretty active there). But has anybody ever
> cared to do something about it?

As far as I'm aware, there has been no particular drive to improve ANSI
compatibility.  That discussion was in particular about the contrib
module, which suffers from lack of maintainers.  I'm of the opinion that
we should drop code that is not actively maintained, but I suspect that
would be unpopular!

> I get so many warnings when compiling
> lwIP that I took the effort changing most of them.

Would you like to submit a patch?

> Though in some cases (mainly pointer arithmetic -> alignment), a fast
> fix is not possible, most warnings are cast issues which could easily be
> fixed.
>
> Anyone else but me cares about that? Hope so...

It's whether they care enough to fix it that counts!  Sadly my
involvement with lwIP is pretty much limited to the mailing lists.  We
rely on bug fixes being submitted by the community of users.

Kieran



_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Compiler warnings

Christiaan Simons
In reply to this post by Goldschmidt Simon
Simon wrote:
>I was just wandering if anybody has looked into ANSI-compatibility for
>the last year...

Not to great lengths, I know this can be improved, same old story:
very little time for that. Fixing big holes in the road has top prio,
solving cold cases and ANSI compat is a bit low prio.

> Kieran, who seems to be pretty active there

Yes, he's doing a great job!

>But has anybody ever
>cared to do something about it?

Really, I do care, and I'm prepared to put in a
little of my time improving this.

> I get so many warnings when compiling
>lwIP that I took the effort changing most of them.

I get rather few warnings on the Unix (1,2?)
and c16x (0) ports, but I've never tried the lwip nios port.
We do have this floating around at Axon, so maybe I can
give it a try. (or at least the toolchain )

I try to minimize warnings to zero if possible,
if only to keep complaints to a minimum.

>Though in some cases (mainly pointer arithmetic -> alignment), a fast
>fix is not possible, most warnings are cast issues which could easily be
>fixed.

>Anyone else but me cares about that? Hope so...

Yes, if you can't create a clean diff for us (against our tree),
then please post your snippets to the savannah bugtracker.

I'll sweep these small fixes before releasing.
(Same thing for the pbuf locking issue)

Tnx for the input.

Christiaan Simons

Hardware Designer
Axon Digital Design

http://www.axon.tv
The information contained in this communication is confidential and is intended solely for the use of the individual or entity to whom it is addressed. Axon Digital Design Group is neither liable for the proper nor for the complete transmission of the information contained in this communication nor for any delay in its receipt. Axon Digital Design Group does not guarantee that the integrity of this communication has been maintained nor that the communication is free of viruses, interceptions or interference. If you are not the intended recipient of this communication, you are hereby notified that reading, disseminating, distributing or copying this message is strictly prohibited. In that case please return the communication to the sender and delete and destroy all copies. In carrying out its engagements, Axon Digital Design Group applies general terms and conditions, which contain a clause that limits its liability. A copy of these terms and conditions is available on request free of charge.

_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

RE: Compiler warnings

Goldschmidt Simon
Christiaan,

I just submitted 2 patches, hope you can include them in CVS. For us,
these patches are quite high prio (below real bugs, of course) since we
are using GCC with -Werror switch turned on. Including these changes in
CVS makes it so much easier to upgrade to a newer version without making
all the changes again on a local version.

The diffs are created against CVS HEAD of 09.10.2006, I hope that's what
you meant with 'our tree'.
I compiled the sources with g++ before and I get a lot more warnings
there, mainly invalid casts. I don't know about other compilers, but at
least gcc does not seem to care about such casts (as long as they are
alignment-compatible). Do you know anything about that (regarding any C
or C++ standard?)

Thanks for caring about this whole issue. (And thanks for working on a
great stack!)

Simon


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

RE: Bug in pbuf.c regarding PBUF_POOL

Goldschmidt Simon
In reply to this post by Kieran Mansley
Hi,

I was just about to submit the patch for using a semaphore, when I
discovered another problem with this. Due to the implementation of
timeouts in lwIP, things can lead to a lockup since sys_sem_wait() may
execute the timeout procedures first.

For me this lead to calling tcp_tmr() when calling pbuf_free() in the
network driver. Problem is that tcp_tmr() calls my netif->output() which
locks itself for exclusive access to the device...

Soooo... I don't currently see a solution for this. Maybe I spend some
time thinking about it...


Simon


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

RE: Compiler warnings

Goldschmidt Simon
In reply to this post by Christiaan Simons
Christiaan (and all others interested in getting clean code :),
 
First, I would like to thank you for the fast integration of parts of
the patch, really nice work. I upgraded to the new version but still got
a few warnings. Apart from a signed/unsigned issue in socket.c
(resulting from sizeof() used in macro on line 90/91 being unsigned
while 'err' in line 94 is of type err_t which is signed), those are
mainly memory-alignment warnings (e.g. casts from u8_t to struct mem* in
mem.c, also in memp.c and pbuf.c).

Have you been able to compile the stack cleanly? If so, what platform
are you using? On a platform like x86 or an 8-bit-platform, those
warnings don't come, of course. Even on our NIOS-II platform, GCC only
generates this warning if the -Wcast-align switch is turned on.
Nevertheless, I think this is a severe warning which should be taken
care of!

The third issue is that GCC warns me about indexing an array with a
'char' in etharp.c (lines 369, 449 & 751). Don't know why this doesn't
work. 'Signed char' and 'int' work, though...

Fourth and last problem is that 'struct sockaddr' and 'struct
sockaddr_in' don't have the same alignment since 'struct sockaddr' only
has u8_t/char members. This can me fixed by declaration 'struct
sockaddr_in' as packed, although I don't see the need for this.

I _could_ submit a patch for the first 2 issues, but I'm not sure what
to do about the etharp-warning and the struct sockaddr_in. Does anyone
have any comments about this?

And for the mem-alignment bugs in mem.c, memp.c and pbuf.c: I don't
think it's the best approach to use u8-arrays for the pools. Better
would be to use arrays of the structs the pools have to use. This way,
the mem-alignment would be taken care of by the compiler (generating the
arrays) instead of the programmer.


Hoping for lots of comments... ;-)

Simon.


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

RE:Bug in pbuf.c regarding PBUF_POOL

Goldschmidt Simon
In reply to this post by Goldschmidt Simon
Hi all,

I thought about this issue some more: Since timeouts don't work with
SYS_LIGHTWEIGHT_PROT==0, I think there should be a warning in opt.h! Or
am I the only one who encounters problems with this? (Might be my
ethernet-driver which corrupts thinks?)

Simon.


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Compiler warnings

Jonathan Larmour
In reply to this post by Goldschmidt Simon
Goldschmidt Simon wrote:

> Christiaan (and all others interested in getting clean code :),
>  
> First, I would like to thank you for the fast integration of parts of
> the patch, really nice work. I upgraded to the new version but still got
> a few warnings. Apart from a signed/unsigned issue in socket.c
> (resulting from sizeof() used in macro on line 90/91 being unsigned
> while 'err' in line 94 is of type err_t which is signed), those are
> mainly memory-alignment warnings (e.g. casts from u8_t to struct mem* in
> mem.c, also in memp.c and pbuf.c).
>
> Have you been able to compile the stack cleanly? If so, what platform
> are you using? On a platform like x86 or an 8-bit-platform, those
> warnings don't come, of course. Even on our NIOS-II platform, GCC only
> generates this warning if the -Wcast-align switch is turned on.
> Nevertheless, I think this is a severe warning which should be taken
> care of!
>
> The third issue is that GCC warns me about indexing an array with a
> 'char' in etharp.c (lines 369, 449 & 751). Don't know why this doesn't
> work. 'Signed char' and 'int' work, though...

It's simply following what the C standard says: "One of the expressions
shall have type ‘‘pointer to object type’’, the other expression shall
have integer type, and the result has type ‘‘type’’."

A simple cast to int (e.g. arp_table[(int)i].state or whatever) may be
sufficient. I've no idea why signed char works though!

> Fourth and last problem is that 'struct sockaddr' and 'struct
> sockaddr_in' don't have the same alignment since 'struct sockaddr' only
> has u8_t/char members. This can me fixed by declaration 'struct
> sockaddr_in' as packed, although I don't see the need for this.

Anonymous unions would be nice, but that's too much of an extension :-).
One solution might be a macro (to be defined in cc.h) to allow alignment,
so in gcc that would be __attribute__((aligned(something))). But I think
an appropriate solution would be to add an element with stronger
alignment, e.g.

struct sockaddr {
   u8_t sa_len;
   u8_t sa_family;
   u16_t sa_dummy;
   char sa_data[12];
};

According to the C standard, that may not strictly be sufficient. But I
think it might work everywhere in practice.

> And for the mem-alignment bugs in mem.c, memp.c and pbuf.c: I don't
> think it's the best approach to use u8-arrays for the pools. Better
> would be to use arrays of the structs the pools have to use. This way,
> the mem-alignment would be taken care of by the compiler (generating the
> arrays) instead of the programmer.

It almost certainly wouldn't save any space though, just make it more
obvious what's going on.

Jifl
--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Bug in pbuf.c regarding PBUF_POOL

Jonathan Larmour
In reply to this post by Goldschmidt Simon
[I saw Simon's recent mail but thought it was better replying to this one...]
Goldschmidt Simon wrote:

> Hi,
>
> 1:
> Sorry to be so 'unprofessional' :)
> But:
> The 1.1.0 version I was using was provided by Altera with the NIOS2
> devkit. I just compared this version to the oringinal 1.1.0 and saw that
> Altera changed that part... They simply just used the semaphore used by
> PBUF_POOL_FREE() in pbuf_pool_alloc() as well. I can submit a patch
> doing this if anyone cares...

I'm relatively new at lwIP and haven't done much with it. But this is my
take from what I've seen so far....

I don't think you can portably wait on a semaphore in pbuf_pool_alloc
which is probably why it isn't. You might be (and probably will be) in a
device driver context and so cannot block.

> 2:
> The other question is: Why did nobody get corrupted pbufs like I do?
> Have you all set SYS_LIGHTWEIGHT_PROT to 1? (since it's by default set
> to 0...)

I don't know the history, but thread-safety is almost certainly why
SYS_LIGHTWEIGHT_PROT even exists. Without it, the code is inherently *not*
thread safe.

> 3:
> OK, maybe the stack is not supposed to be multithreaded, BUT: I have a
> separate thread for receiveing,

As does SLIP and PPP, so this would be a wider stack issue.

> and that thread must allocate fresh
> pbufs (PBUF_POOL in my case) so where should I do that if not in a
> seperate thread? With SYS_LIGHTWEIGHT_PROT=1, INTs are diables (=
> threadsafe); in PBUF_POOL_FREE(), a semaphore is used (=threadsafe)

With SYS_LIGHTWEIGHT_PROT=1, semaphores are not used in PBUF_POOL_FREE().
Just SYS_ARCH_PROTECT() etc.

>, so
> I still think the design goal is to make pbufs and memory access thread
> safe!
>
> 4:
> The reason pbuf_pool_free_lock is never set to anything except 0 is that
> it is not used on the FREE-side. What strikes me more is, that
> pbuf_pool_alloc_lock is never tested against, only set to a value. But
> that seems only to be a typo (in v1.1.1, pbuf.c line 146 should use
> pbuf_pool_alloc_lock instead of pbuf_pool_free_lock).

Although there is a problem here, I don't believe that is a typo. I
believe pbuf_pool_alloc was originally written expecting to be called from
drivers potentially in an interrupt and can't block. I believe that is the
expected model and is probably an important thing to preserve. And
pbuf_pool_free is only expected to be called from thread-level stack code.

So with that model an alloc doesn't have to worry about interrupting
another alloc, only a free. And if it finds it is interrupting a free, it
should just bomb out and return no buffer.

But first of all, indeed nothing increments pbuf_pool_free_lock. And
second of all, there are calls to pbuf_alloc for allocations from
PBUF_POOL in other places that may themselves then be interrupted by a
driver interrupt:

$ egrep -r 'pbuf_alloc.*POOL' .
./core/ipv4/ip_frag.c:      p = pbuf_alloc(PBUF_LINK, ip_reasslen, PBUF_POOL);
./core/pbuf.c:        q = pbuf_alloc(PBUF_RAW, p->len, PBUF_POOL);
./netif/ppp/ppp.c:      tb = pbuf_alloc(PBUF_RAW, 0, PBUF_POOL);
./netif/ppp/ppp.c:      headMB = pbuf_alloc(PBUF_RAW, 0, PBUF_POOL);
./netif/ppp/ppp.c:      headMB = pbuf_alloc(PBUF_RAW, 0, PBUF_POOL);
./netif/ppp/ppp.c:                    nextNBuf = pbuf_alloc(PBUF_RAW, 0,
PBUF_POOL);
./netif/ppp/vj.c:               np = pbuf_alloc(PBUF_RAW, n0->len +
cs->cs_hlen, PBUF_POOL);
./netif/ppp/vj.c:               np = pbuf_alloc(PBUF_RAW, cs->cs_hlen,
PBUF_POOL);
./netif/slipif.c:        p = pbuf_alloc(PBUF_LINK, PBUF_POOL_BUFSIZE,
PBUF_POOL);


So the (likely) intended form of protection is bogus. You can't shield an
allocation from another allocation. As both would set pbuf_pool_alloc_lock
to 1 and you couldn't tell which one "won". If you instead tries to do
"pbuf_pool_alloc_lock++" that wouldn't help as that is not guaranteed
atomic (and usually isn't).

The only solutions I can think of without !SYS_LIGHTWEIGHT_PROT would
involve adding new elements to the sys_arch abstraction. In which case you
may as well just implement SYS_LIGHWEIGHT_PROT.

With the current code, I think the only solution is SYS_LIGHTWEIGHT_PROT.
My opinion is that the !SYS_LIGHTWEIGHT_PROT stuff should probably just be
removed.

Unless someone knows that the above allocs could avoid using PBUF_POOL.
Why they're being allocated from there is a bit of a mystery to me.

Then again the above might be complete rubbish.

Jifl
--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Bug in pbuf.c regarding PBUF_POOL

Kieran Mansley
On Fri, Oct 20, 2006 at 11:37:23PM +0100, Jonathan Larmour wrote:
> With the current code, I think the only solution is SYS_LIGHTWEIGHT_PROT.
> My opinion is that the !SYS_LIGHTWEIGHT_PROT stuff should probably just be
> removed.
>

Thanks for the detailed post - good to see folks getting to grips with
the stack.  The above seemed to summarise quite nicely and I agree
completely with that.

Kieran


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

RE: Compiler warnings

Goldschmidt Simon
In reply to this post by Jonathan Larmour

> > The third issue is that GCC warns me about indexing an array with a
> > 'char' in etharp.c (lines 369, 449 & 751). Don't know why
> this doesn't
> > work. 'Signed char' and 'int' work, though...
>
> It's simply following what the C standard says: "One of the
> expressions shall have type ''pointer to object type'', the
> other expression shall have integer type, and the result has
> type ''type''."
>
> A simple cast to int (e.g. arp_table[(int)i].state or
> whatever) may be sufficient. I've no idea why signed char
> works though!
>

I found that out by now: C standard does not define 'char' to be signed
or unsigned but leaves that to the compiler to decide. 'signed char' and
'unsigned char' both worked for me...

> > Fourth and last problem is that 'struct sockaddr' and 'struct
> > sockaddr_in' don't have the same alignment since 'struct sockaddr'
> > only has u8_t/char members. This can me fixed by
> declaration 'struct
> > sockaddr_in' as packed, although I don't see the need for this.
>
> Anonymous unions would be nice, but that's too much of an
> extension :-).
> One solution might be a macro (to be defined in cc.h) to
> allow alignment, so in gcc that would be
> __attribute__((aligned(something))). But I think an
> appropriate solution would be to add an element with stronger
> alignment, e.g.
>
> struct sockaddr {
>    u8_t sa_len;
>    u8_t sa_family;
>    u16_t sa_dummy;
>    char sa_data[12];
> };
>
> According to the C standard, that may not strictly be
> sufficient. But I think it might work everywhere in practice.
>

I tried it with __attribute__((packed)) and that worked, although the
layout (and alignment) of the struct does effectively _not_ get changed.
Might be flaw in GCCs warnings?

> > And for the mem-alignment bugs in mem.c, memp.c and pbuf.c: I don't
> > think it's the best approach to use u8-arrays for the pools. Better
> > would be to use arrays of the structs the pools have to
> use. This way,
> > the mem-alignment would be taken care of by the compiler
> (generating
> > the
> > arrays) instead of the programmer.
>
> It almost certainly wouldn't save any space though, just make
> it more obvious what's going on.
>

Of coures this doesn't save space, but it makes the code cleaner. With
the current code, you either get a warning when casting, or you cast
yourself by using MEM_ALIGN(x). In both cases, when changing the code,
you lose the compiler-check if the resulting pointer is really aligned
correctly!


Simon


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: Compiler warnings

Christiaan Simons
In reply to this post by Goldschmidt Simon
Simon,

>I upgraded to the new version but still got a few warnings.
> Apart from a signed/unsigned issue in socket.c
>(resulting from sizeof() used in macro on line 90/91 being unsigned
>while 'err' in line 94 is of type err_t which is signed), those are
>mainly memory-alignment warnings (e.g. casts from u8_t to struct mem* in
>mem.c, also in memp.c and pbuf.c).

Noted. Didn't accept all changes, wasn't sure about a few
and I hate introducing typecasts or unsized types for sake of
style and portability (what is sizeof(int) or what is MAX_INT?).
Some lines are going to change anyway.
Hope I can find a few minutes this evening to work on mem.c.

>Have you been able to compile the stack cleanly?

More or less cleanly, still get a few warnings.
Some of these are actually portability problems, no ANSI C issues,
or things I that I consider to be on my todo list.
(clock_t type, some 64 bit arithmatic in perf.c not possible
on embedded archs, need to complete SNMP TCP connection table etc)

>If so, what platform are you using?

Currently using OpenBSD3.9 on i386 gcc 3.3.5 plus OpenBSD patches.
I've also tried a bleeding-edge Gentoo, with a bleeding-edge GCC 4.1.1,
which results in a horriffic amount of warnings on struct packing. :-(

Also using the c16x / Tasking port, without warnings,
only using the 'core' and 'contrib' files.
(but I don't trust this compiler error reporting capability)

> -Wcast-align switch is turned on.
>Nevertheless, I think this is a severe warning which should be taken
>care of!

Hmm. Doesn't -Wall -pedantic turn this on?

>The third issue is that GCC warns me about indexing an array with a
>'char' in etharp.c (lines 369, 449 & 751). Don't know why this doesn't
>work. 'Signed char' and 'int' work, though...

Simply be more explicit in cc.h, typedef your s8_t to signed char.

>And for the mem-alignment bugs in mem.c, memp.c and pbuf.c: I don't
>think it's the best approach to use u8-arrays for the pools. Better
>would be to use arrays of the structs the pools have to use. This way,
>the mem-alignment would be taken care of by the compiler (generating the
>arrays) instead of the programmer.

Thanks for hinting.

>Hoping for lots of comments... ;-)

Enough?

Christiaan Simons
Hardware Designer
Axon Digital Design
http://www.axon.tv
The information contained in this communication is confidential and is intended solely for the use of the individual or entity to whom it is addressed. Axon Digital Design Group is neither liable for the proper nor for the complete transmission of the information contained in this communication nor for any delay in its receipt. Axon Digital Design Group does not guarantee that the integrity of this communication has been maintained nor that the communication is free of viruses, interceptions or interference. If you are not the intended recipient of this communication, you are hereby notified that reading, disseminating, distributing or copying this message is strictly prohibited. In that case please return the communication to the sender and delete and destroy all copies. In carrying out its engagements, Axon Digital Design Group applies general terms and conditions, which contain a clause that limits its liability. A copy of these terms and conditions is available on request free of charge.

_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
123