[icecast-dev] ICECAST enseirb group

[Michael Smith]

Ok, lots of good questions here, I'll try and respond to them...

> > On Sun, Mar 09, 2003 at 07:47:02PM +0100, Arnaud Ebalard wrote:
> > > Hello, everybody, here is the ENSEIRB group (Sébastien, Florian,
> > > Jérémy, Denis, Mickaël, Antoine, Guiilaume and me).
> > > We are trying to understand in details how ICECAST server works. The
>
> reading
>
> > > of sources is quite difficult for the numerous parts that come with no
> > > comments. We have some questions that arose at the reading of sources.
>
> There
>
> > > are probably many stupid things or misunderstandings in our questions.
> > > Excuse us for those one. We are waiting for answers before beginning
>
> working
>
> > > on the sources. Let us know your ideas about the project, the
> > > directions
>
> you
>
> > > want to follow and the parts you are working on.
> > >
> > > ----------------------------------------
> > > THE QUESTIONS :
> > >
> > > In file connection.c, the couple of functions _add_connection and
> > > _get_connection work on the list structure _queue : insertion of a
> > > connection by mean of _add_connection is made in a single operation
>
> while
>
> > > getting a connection by mean of _get_connection is made by scanning the
> > > whole list to find the oldest connection in the list. This seems to be
> > > hugely inefficient and quite blocking for other threads that are trying
>
> to
>
> > > access the list simultaneously. A structure of circularly
> > > single-chained list could be more efficient : insertion of an element
> > > and access to the last element can be done in a single operation.

This is true - the current implementation here is not particular efficient. 
This could be solved very simply by just keeping a second pointer to the end 
of the queue, or in any number of other ways.

I'd be happy to apply a patch to implement this, but I'm not going to go to 
the effort of doing it myself - this is NOT something where efficiency 
matters at all - the queue will most often be of length zero or one. Longer 
queues are possible but unlikely to happen frequently.

> > >
> > > ----------------------------------------
> > >
> > > We saw that the stat module worked with an avl in order to keep the
>
> global
>
> > > parameters ( _stats.global_tree ) and "per source" parameters (
> > > _stats.source_tree ) : we don't understand the use of an avl for the
>
> global
>
> > > parameters. Can't we directly use fields in the _stats_t structure
>
> instead
>
> > > of an avl. It would be more efficient than searching the avl for a key
>
> and
>
> > > the asociated value. The values are saved as strings in the avls : so
> > > we keep converting strings to integer, incrementing them, and then
>
> reconverting
>
> > > them to srings like in the function stats_event_inc. Can this be
>
> improved or
>
> > > is there anything we didn't understood or saw ?

The stats_t structure doesn't have any fields (apart from pointers to the avl 
trees). The design is like this to allow any sort of stats to be added to the 
tree without having to have them registered in a central place (in stats.h). 
This is a debatable decision, and the design does have some performance 
implications (like the string->integer->string conversions), but I don't 
think they're serious. 

The stats code generally needs a moderate amount of attention - it's not used 
too consistently, among other things. Designing better ways to do this would 
be worth considering.

<p>> > >
> > > ----------------------------------------
> > >
> > > In _handle_connecion(), after stats_event_inc(NULL, "connections"); we
>
> make
>
> > > sock_set_blocking(con->sock, SOCK_BLOCK); and then use
> > > util_read_header(con->sock, header, 4096) in order to read the header
> > > of
>
> the
>
> > > request send by the client. Why putting the socket in SOCK_BLOCK mode ?
>

The header-reading code is not prepared to deal with non-blocking I/O, so it 
has to be in blocking mode. 

In the current design (this could be reconsidered and perhaps changed), all 
connections are handled initially in blocking mode, then when handoff to a 
source happens (for client connections, and/or for file-serving connections, 
but not for any other sort), they're flipped back to non-blocking mode. This 
design decision was made as a compromise between scalability/efficiency and 
simpliciity of implementation. The vast majority of client connection time is 
generally spent in non-blocking mode being handled by a single source thread, 
so this is not unreasonable.

> Why
>
> > > receiving data from the socket byte per byte ? We know there can be
> > > something behind the header that we would read on the socket if we
>
> receive
>
> > > more than one byte at once : can't we get the complete request of the
>
> client
>
> > > (header + data), and then parse this to find the '\n\n' (end of the
>
> header)
>
> > > and suppress '\r' ?

This is a good suggestion, but would require a generic buffering layer in the 
socket I/O code. This hasn't been done, but certainly could be. Again, this 
is a minor inefficiency where the additional CPU load fades into 
insignificance against the bulk of the work done. Optimising things with no 
visible cpu impact is lower-priority (at least for the active developers) 
than bugfixes or adding neccesary features.

> > >
> > > ----------------------------------------
> > >
> > > In function _handle_conection in file connection.c, the buffer has a
>
> size of
>
> > > 4096 bytes. Is there any particular reason for this, why not use a
>
> constant
>
> > > macro ?

This constant is fairly arbitrary. It could be a named (#defined) constant, 
there's no good reason for it not being so.

> > >
> > > ----------------------------------------
> > >
> > > In _slave_thread function, there is :
> > >
> > > ...
> > > while (_initialized)
> > > {
> > >  if (max_interval > ++interval)
> > >    {
> > >     thread_sleep(1000000);
> > >     continue;
> > >    }
> > >  else
> > > ...
> > >
> > > Can't we do something like :
> > > ...
> > > while (_initialized)
> > > {
> > >  if (max_interval > interval)
> > >    {
> > >     thread_sleep(1000000 * (max_interval - interval));
> > >    }
> > >  else

No. The sleep is kept short so that in _initialized goes to zero (as on a 
shutdown request), the loop exits reasonably quickly. The ideal 
implementation uses signals to break out of the sleep system call rather than 
relying on this, but relying on correct behaviour in a program that mixes 
threads and signals tends to cause significant portability issues (whilst 
it's clearly specified, non-compliant implementations are very common).

<p>> > > ...
> > >
> > > ----------------------------------------
> > >
> > > In fserv_thread_function() function in fserve.c file, can
> > > avl_tree_unlock(client_tree); be made just after client_node =
> > > avl_get_first(client_tree); instead of being done at the end or in the
> > > if(!client_node) { .... } block ? Are we wrong ? If yes, why ?

Looks like it could be. I wrote that code at about 2am one night, it's far 
from clear. They're more or less equivalent, though - there can be no 
contention for the client lock there, so it doesn't matter (come to think of 
it.. I'm not entirely sure why I'm bothering to use locks on the client tree 
there at all - they should only be needed on the pending tree. The same is 
probably also true of the client_tree for each of the sources - actually, no, 
the admin stuff needs to be able to grab read locks here). 

This is a good point, I'll take a good look at it in detail when I have a 
little more time, and when I'm more awake.

> > >
> > > ----------------------------------------
> > >
> > > In fserv_thread_function() function in fserve.c file, can something
>
> wrong
>
> > > happen if we are between avl_tree_unlock(pending_tree); and
> > > avl_tree_unlock(client_tree); in :
> > >
> > > ...
> > > avl_tree_unlock(pending_tree);
> > > avl_tree_unlock(client_tree);
> > > thread_cond_wait(&fserv_cond);
> > > continue;
> > > ...
> > >
> > > if another thread add a client in the pending three before this thread
> > > reaches the thread_cond_wait(&fserv_cond); line ? Could a client be
>
> trapped
>
> > > and not handled because this 3 lines are not an atomic action ?

Yes. Good catch. Obviously a very unlikely case, but this should be fixed. The 
client won't be permanently trapped even in this case - only trapped until 
the next file-serving client is added. 

This requires a fair bit of work in the avl and thread modules, I think. 
Unless.. it might be ok to just throw away the rwlocks here (there aren't 
multiple readers) and switch to using mutexes. Still requires some work in 
the thread module. 

<p>> > >
> > > ----------------------------------------
> > >
> > > When using the select() function, we always calculate the biggest value
>
> for
>
> > > socket descriptor in order to pass it as first argument. Why don't we
>
> use
>
> > > FD_SETSIZE ? Is that more efficient or is there another reason to keep
> > > calculate the biggest socket descriptor ?

This is more efficient, and is the "correct" way to use select(). poll() is 
used in preference to select anyway. Some efficiency work would be nice with 
both of these, though, so that the poll array or select bitset is kept around 
rather than being recreated every time.

> > >
> > > ----------------------------------------
> > >
> > > This is a big question for us : we saw the use of avl in order to store
>
> all
>
> > > the clients (in client_tree ). The thing we also noticed is that all
> > > the accesses we saw where made using avl_get_first() and
> > > avl_get_next(). We didn't saw any other access made by using a key or ,
> > > for example, the connection id. Wouldn't it be better to use a simple
> > > circularly linked
>
> list
>
> > > : the accesses and insertions would be faster (constant!). Are we wrong
>
> ? If
>
> > > yes, why ?

As things stand currently, the use of avl trees here is unneccesary. However, 
there will be functionality fairly soon (for admin usage) where lookup for 
key or connection id will be important.

> > >
> > > ----------------------------------------
> > >
> > > In sock_write_bytes(), there is a "sanity check" which can return
> > > SOCK_ERROR. If this happens, there can be a problem in the calling
>
> function
>
> > > because this one will then verify the kind of return using
> > > sock_recoverable(sock_error()). And sock_error() will return something
>
> wrong
>
> > > because of errno or WSAGetLastError() are not concerned by the return
> > > of
>
> the
>
> > > "sanity check".

Not sure what you mean here - sock_error() correctly returns errno, which is 
what sock_recoverable() is designed to use. The return value is either 
positive (normal case) or SOCK_ERROR indicating that some error occurred. 
This is perfectly self-consistent. Can you explain in more detail why you 
think this is wrong?

> > >
> > > ----------------------------------------
> > >
> > > In fserv_thread_function , there is a first scan of the client tree
>
> (with
>
> > > avl_get_first(client_tree) ), in order to serve the client. In the same
>
> time
>
> > > the value of client->con->error is set to 1 if the client was
> > > completely served (end of file). Then, there is another complete scan
> > > of the client_tree in order to delete all the node for which the client
> > > has got
>
> a
>
> > > client->con->error set to 1. This is time consuming. If we used a list
>
> (as
>
> > > we said before) to store the clients we could efficiently do the
> > > service
>
> of
>
> > > the clients and, in the same time, delete the completely served client.

Yes. The fileserving code was written late at night, it's not exactly the most 
efficient stuff I've ever written. However, again, the tree lookups are more 
efficient when we need to do other stuff (like the admin interface saying 
"remove this client") - though that's not complete yet, the underlying system 
is designed to deal with this case efficiently.

> > >
> > > ----------------------------------------
> > >
> > > Can someone explain us the strategy of source_find_mount, and
>
> particularly
>
> > > the aim of that line :
> > >
> > > ...
> > > node = global.source_tree->root->right;
> > > ...
> > >
> > > Why not using source_compare_sources ?

Not sure, I didn't write that bit. The code looks correct and reasonably 
efficient, though. 

> > >
> > > ----------------------------------------
> > >
> > > Where does the avl module come from ? What are his performances ?
> > > possibilities ? limitations ?

Maybe Jack can answer this one? I don't know much detail about the avl 
implementation, other than how to use them.

> > >
> > > ----------------------------------------
> > >
> > > What can ICECAST do with *.m3u files ?

Serve them (via the fileserving functionality), and autogenerate them for 
files and sources.

> > >
> > > ----------------------------------------
> > >
> > > This is probably a quite stupid question but I'd like to know why
>
> ICECAST
>
> > > isn't a multiprocessing application? The main problem is that global
> > > variables can be easily used in multithread applications but with
> > > difficulties in multiprocessing applications if modules are quite bound
> > > together. In ICECAST, something could probabaly be done to make the
>
> fserve
>
> > > module and the source module run in differents processes. This would be
> > > great for Multiprocessing Systems to separate this to service on 2
> > > processors. We could serve more clients at the same time ( Am I wrong
>
> ? ).
>
> > > Is the limitation induced by bandwith on the server and not on CPU ?

Icecast is heavily multithreaded - the threads can and will run on different 
cpus if you have a multi-CPU machine. So yes, you're wrong.

Additionally, you'd be hard-pressed to find a system where you were cpu-bound 
due to icecast, and not network-bound. icecast is quite capable (assuming 
your network hardware is) of saturating even gigabit ethernet without the cpu 
usage of icecast itself becoming excessively high (however, the cpu usage of 
the kernel-level networking implementation will tend to get pretty high in 
this case).

<p>Mike

--- >8 ----
List archives:  http://www.xiph.org/archives/
icecast project homepage: http://www.icecast.org/
To unsubscribe from this list, send a message to 'icecast-dev-request at xiph.org'
containing only the word 'unsubscribe' in the body.  No subject is needed.
Unsubscribe messages sent to the list will be ignored/filtered.