[patch #5785] Integrate SNMP initialization in tcpip.c

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

URL:
  <http://savannah.nongnu.org/patch/?5785>

                 Summary: Integrate SNMP initialization in tcpip.c
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: fbernon
            Submitted on: mardi 06.03.2007 à 20:50
                Category: None
                Priority: 5 - Normal
                  Status: In Progress
                 Privacy: Public
             Assigned to: fbernon
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any

    _______________________________________________________

Details:

With the very useful Christiaan Simons's SNMP Agent, we can add this protocol
to our products.

I propose to help SNMP integration for new users like this :

#if LWIP_SNMP
static void
snmp_timer(void *arg)
{ snmp_inc_sysuptime();
  sys_timeout( SNMP_TMR_INTERVAL, snmp_timer, arg);
}
#endif /* LWIP_SNMP */

static void
tcpip_thread(void *arg)
{
  struct tcpip_msg *msg;

#if LWIP_SNMP
  snmp_init();
#endif /* LWIP_SNMP */

#if IP_REASSEMBLY
  sys_timeout( IP_TMR_INTERVAL, ip_timer, NULL);
#endif
  sys_timeout( ARP_TMR_INTERVAL, arp_timer, NULL);

#if LWIP_SNMP
  sys_timeout( SNMP_TMR_INTERVAL, snmp_timer, NULL);
#endif // LWIP_SNMP

//...

So, before call tcpip_init(), new users just have to call :

#if LWIP_SNMP
snmp_set_sysdesr(sysdescr_default,&sysdescr_len_default);
snmp_set_syscontact(syscontact_default,&syscontact_len_default);
snmp_set_sysname(sysname_default,&sysname_len_default);
snmp_set_syslocation(syslocation_default,&syslocation_len_default);
//etc...
#endif

More, why don't integrate in tcpip.c a new "tool" function (with a name like
"lwip_init" or other) which do :

void lwip_init()
{ #if LWIP_STATS
  stats_init      ();
  #endif /* STATS */
  #if (NO_SYS == 0)
  sys_init        ();
  #endif /* (NO_SYS == 0) */
  mem_init        ();
  memp_init       ();
  pbuf_init       ();
  etharp_init     ();
  netif_init      ();
  lwip_socket_init();
}






    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #1, patch #5785 (project lwip):

Hm, I don't like the way SNMP gets it's sys_uptime anyway (since it produces
high system load calling snmp_inc_sysuptime even if you have no client which
reads it). AND it's not very accurate if called by an lwIP timeout timer (as
stated in discussion to bug #19167).

If we do the timeouts using a timestamp, we can get the sys_up_time when the
client reads it, that would be more accurate.

I agree with you we need an lwip_init() init function. But I think it would
be better not to integrate it in tcpip.c. I think this should also be
available to raw-only users. We could make the right initialization for both
configurations.
Having a function like lwip_init() would definitively help new users!

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #2, patch #5785 (project lwip):

Ye,s agree with you, timeout timer are not a perfect feature, and, in this
case, I will prefer to be able to remove "snmp_inc_sysuptime()" and the
variable "sysuptime" and to be able to define my "own" function
"snmp_get_sysuptime" (based on platform timer). So, overhead will be reduce.

About lwip_init() function, in a lwip.c file? Perhaps it will be usefull to
add a define to tell is tcpip.c is used, to don't increase footprint if not
necessary (sockets.c, api_lib.c, api_msg.c, tcpip.c). With that, it will give
a pseudo-code like for tcpip.c users:

main()
{ //TODO: Target specific init

  lwip_init();

  //TODO: Add here network inferfaces with net_add

#if LWIP_SNMP
  snmp_set_sysdesr(...);
  snmp_set_syscontact(...);
  snmp_set_sysname(...);
  snmp_set_syslocation(...);
  //etc...
#endif


  tcpip_init();

  //TODO: Application
}  

For RAW API users, only tcpip_init will have to be replace...


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #3, patch #5785 (project lwip):

I think adding the lwip_init function might cause more problems than it
solves, as many people wont want or need to initialise all the modules -
particularly if using the raw API.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #4, patch #5785 (project lwip):

Agree with you Kieran about functions called by lwip_init. In the current
options availables, we can't know is we are using the tcpip.c thread. I think
that adding a new option (yes, I know, another one), it will be possible to
remove any extra code or initialization which are not used per rawapi
users...

But, if I don't do any mistake, using tcpip.c need to use all that:

#if LWIP_STATS
stats_init ();
#endif /* STATS */
#if (NO_SYS == 0)
sys_init ();
#endif /* (NO_SYS == 0) */
mem_init ();
memp_init ();
pbuf_init ();
etharp_init ();
netif_init ();
lwip_socket_init();

lwip_socket_init don't have to be used for rawapi, but it seems that all
others are need. Wrong?



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #5, patch #5785 (project lwip):

I think this is a problem that would be best solved with documentation rather
than adding an extra function.  That way the developers will be able to make
the right decision about which modules to initialise, and the documentation
could include your suggested function as a sensible default.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #6, patch #5785 (project lwip):

Ok, right, documentation is important.

If I provide some documentation "drafts", is there people ready to correct
them? In a first time, I propose :

- "Getting Started lwIP with multithread operating system"
- "Tunning lwIP options"

All that, based on previous threads in forum...



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #7, patch #5785 (project lwip):

To reduce timer overhead for snmp_get_sysuptime, I propose to write something
like that :

in snmp.h :

#if (sys_get_tickcount==0)
void snmp_get_sysuptime(u32_t *value);
#else
#define snmp_get_sysuptime(t) sys_get_tickcount(t)
#endif /* (sys_get_tickcount==0) */

in mib2.c :

#if (sys_get_tickcount==0)
/**
 * Must be called at regular 10 msec interval from a timer interrupt
 * or signal handler depending on your runtime environment.
 */
void snmp_inc_sysuptime(void)
{
  sysuptime++;
}

void snmp_get_sysuptime(u32_t *value)
{
  *value = sysuptime;
}
#endif /* (sys_get_tickcount==0) */

// There is also a little patch to do in system_get_value to use the function
and not directly sysuptime variable/


in opt.h :

#ifndef sys_get_tickcount
#define sys_get_tickcount 0
#endif

So, if we already get a target "tickcount", we can write in lwipopts.h :

#define sys_get_tickcount(t) sys_arch_get_tickcount(t)

It would be better (no overhead, only call when SNMP are process, and more
accurate, because not based on sys_timeout).

Your comments?


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #8, patch #5785 (project lwip):

That's a good idea, but I would like to postpone that from CVS HEAD. I've
made a new branch DEVEL_SOCKET, where I'm working on socket stability
(multithreading stability, so netconn_ API also).

(I'd taken the DEVEL branch but that was out of date and I don't know if I
can use it or if someone else is 'working' with that.)

I also want to include the timestamp thing in that branch, to test it without
releasing it to HEAD. Maybe you can check it in there?

Oh, regarding coding style: I would favor a define in upper case letters...

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #9, patch #5785 (project lwip):

I think your idea to use another CVS branch for features like timestamps is
good, but multithread stability and netconn api are more features to add to
HEAD. Except, of course, if you use timestamps for that (is it?). But this is
my point of view....

Can you tell me more about what you want to do in this new branch? I have an
idea, from the bug# 19167, but, what else? (to avoid to do the same job
twice...). In fact, the idea is the same than Dmity's sys_arch_time_now, but
I don't have read all the subject (I do it now)...

>Oh, regarding coding style: I would favor a define in upper case letters..

Right, but it was to be closer that sys_ functions names...



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #10, patch #5785 (project lwip):

Just wanted to have a "testing" branch, so that we only merge changes into
HEAD if we are sure they work... I know DEVEL_SOCKET is a bad name...

It was Kieran's idead to not commit everything to HEAD and I must say it's
the right thing, since HEAD is still kind of what we 'release' to the public,
isn't it?

I wanted to check in the timestamp stuff from Dmitry in that branch, also.
(as I said, DEVEL_SOCKET is a bad name.)

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #11, patch #5785 (project lwip):

> It was Kieran's idead to not commit everything to HEAD

Yes, that just I wanted to say. But you're right. If you want some help to
test and check some code about multithread, "call" me, I will be please to
spend time on DEVEL_SOCKET branch (you'are also right about the name :) ).





    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #12, patch #5785 (project lwip):

I think there are two alternatives:
1. to add get_arch_time_now() as described in #19167:
2. to add sys_arch_get_tickcount() and two functions (or macros) for
conversation ticks to or from milliseconds.

The problem with the first approach is implementation of get_arch_time_now(),
which should wrap correctly over its maximum value (as I decribed in #19167).

In the second case, we store timestamps in ticks, so converation ticks to
milliseconds is performed only for timeout values (which are relatively
small), so there is no problem with overflow when you calculate milliseconds
by formula (ticks*M/N). And the tick counter wraps naturally without any
effort (you just increment it by one in the timer interrupt). So for those
who makes a port, #2 is simpler and more straightforward.



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #13, patch #5785 (project lwip):

Just an alternative idea: most of "timestamp" features are implemented as
"unsigned int 64bits". With that, you can use it like "absolute" time (like
file times, "number of 100-nanosecond intervals since January 1, 1601"), or
relative time from boot time.

So, Solution #1, why not "u64_t sys_arch_get_time_now();" ? Is it a real
problem? If a target can provide a u64_t tickcount, it can  always use the
older system with timeout...

About SNMP problem. sysUpTime is defined like this:

http://www.faqs.org/rfcs/rfc1213.html "RFC 1213 - Management Information Base
for Network Management of TCP/IP-based internets:MIB-II"

          sysUpTime OBJECT-TYPE
              SYNTAX  TimeTicks
              ACCESS  read-only
              STATUS  mandatory
              DESCRIPTION
                      "The time (in hundredths of a second) since the network
management portion of the system was last                    
re-initialized."
              ::= { system 3 }

http://www.faqs.org/rfcs/rfc1155.html "RFC 1155 - Structure and
identification of management information for TCP/IP-based internets"

 TimeTicks ::=
                      [APPLICATION 3]
                          IMPLICIT INTEGER (0..4294967295)

All that indicate that sysUpTime will overflow each ~497 days. Long, but not
infinite. So, Solution #2, if we use a u32_t tickcount (in milliseconds) to
get sysUptime, ok, it will overflow each ~49.7 days, but is it a real
problem? Most of time, SNMP supervisors only display this information, but
there's no processing on it...

Last, Solution #3, is perhaps to keep the actual sys_timeout solution, but
with another period. We could call snmp_inc_sysuptime only each 1000ms (by
example), write something like that:

void snmp_inc_sysuptime( u32_t inc)
{ sysuptime+=inc;
}

Less accurate, but less overhead...


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #14, patch #5785 (project lwip):

Hm, 64 bit is what I would favour, also. But 64-bit calculating in on a
16-bit processor is not what you can call fast, I think! Plus we need that
timestamp rather often...

But I would favour a portable solution (like a sys_time_t) which allows for
64bit or 32bit time (16bit is too short). I think Dmitrys patch can do that.

Regarding the SNMP sysUpTime value: if it is defined to be in 100th of
seconds (not 1000th), we should implement it that way. Of course, (also with
the current solution?) if you count the time in ms and the nconvert to 10ms,
you get an overflow after 49,7 days instead of 497 days. I don't think that's
a real problem. As you say, "not infinite"...

I would definitively not change SNMP time implementation until that timestamp
thing is solved. (sysuptime+=inc would work but is a suboptimal solution, I
think.)

This leads us to bugs #19167 and #1902:

Should we implement this / check it in to CVS HEAD or not???? It will break
current ports, but I think it's worth it!

Kieran?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #15, patch #5785 (project lwip):

I don't have enough understanding of the many timeout related issues that are
going on at the moment to make a good decision I'm afraid. However, I don't
want this to hold things up and I'm happy for you (collectively) to make a
decision if some consensus can be reached.  I would prefer:
 - a complete solution, even if that breaks ports - we're breaking ports in
the next release anyway, so makes sense to get all the breakage out in one
go.
 - something that will work well for both the raw API and the
sequential/sockets API
 - something that is efficient to execute and port.
 - something that doesn't bring too much detail of "other" protocols into the
core of lwIP.

Your use of a branch to share big changes such as this until they have been
tested is much appreciated.  

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #16, patch #5785 (project lwip):

I don't like 64-bit in lwIP core, for the following reasons:
 - it introduces unnecessary overhead
 - there are still compilers that do not support 64-bit.

I believe it is important to decide what new function we want to add in
ports, whether it is get_tickcount or time_now.

I prefer get_tickcount because it is easier to implement in any port without
having any problem with overflow.


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #17, patch #5785 (project lwip):

> "to add sys_arch_get_tickcount() and two functions (or macros) for
conversation ticks to or from milliseconds. "

To avoid these kind of problems, and got a ms value, I use functions like
this (is it in the same spirit of your idea???) :

/*-----------------------------------------------------------------------------------*/
/* Public sys_arch tool */
/* Returns current tickcount */
u32_t sys_start_tickcount()
{ UInt32 ulTickCount = 0;
  tmbslCoreGetTickCount(&ulTickCount);  
  return ulTickCount;
}

/*-----------------------------------------------------------------------------------*/
/* Public sys_arch tool */
/* Returns current number of milliseconds from "tickcount" parameter */
u32_t sys_stop_tickcount( u32_t tickcount)
{ UInt32 ulTickCount = 0;
  tmbslCoreGetTickCount(&ulTickCount);  
  if (ulTickCount>=tickcount)
   { return ((ulTickCount-tickcount)             /ulTicksPerMilliSec);
   }
  else
   { return (((0xffffffff-tickcount)+ulTickCount)/ulTicksPerMilliSec);
   }
}

I don't use that, but I suppose it will help :

/* Public sys_arch tool */
/* Returns current number of milliseconds between "old" and "new" parameters
*/
u32_t sys_diff_tickcount( u32_t oldtickcount, u32_t newtickcount)
{ if (newtickcount>=oldtickcount)
   { return ((newtickcount-oldtickcount)             /ulTicksPerMilliSec);
   }
  else
   { return (((0xffffffff-oldtickcount)+newtickcount)/ulTicksPerMilliSec);  
}
}



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #18, patch #5785 (project lwip):

> To avoid these kind of problems, and got a ms value

It is enough to have sys_arch_get_tickcount() and sys_diff_tickcount().

>  return (((0xffffffff-tickcount)+ulTickCount)/ulTicksPerMilliSec);

This is incorrect. Let's suppose tickcount=0xffffffff and ulTickCount = 0
then your formula gives 0 when the correct answer is 1.




    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



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

[patch #5785] Integrate SNMP initialization in tcpip.c

Ondrej Lufinka

Follow-up Comment #19, patch #5785 (project lwip):

Right, except that any difference < ulTicksPerMilliSec will give 0 ms.

;) <joke>


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5785>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



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