[SR-Dev] what if ...

[Andrei Pelinescu-Onciul]

On May 21, 2009 at 13:22, Daniel-Constantin Mierla <miconda at gmail.com> wrote:
> 
> 
> On 05/21/2009 12:10 PM, Jan Janak wrote:
> > On 20-05 19:44, Daniel-Constantin Mierla wrote:
> >   
> >> On 05/20/2009 07:31 PM, Andrei Pelinescu-Onciul wrote:
> >>     
> >>> On May 20, 2009 at 19:24, Daniel-Constantin Mierla <miconda at gmail.com> wrote:
> >>>   
> >>>       
> >>>> Hello,
> >>>>
> >>>> what would be the drawback of having sip_msg being all the time in 
> >>>> shared memory? Would pkg vs shm operations have relevant impact?
> >>>>     
> >>>>         
> >>> Yes.
> >>>
> >>>   
> >>>       
> >>>>  From personal observations, most of the requests (over 95%) end in TM 
> >>>> module (to absorb retransmission or to forward) where the sip_msg s 
> >>>> moved to shm. It would make things simpler for tm callbacks and related 
> >>>> routes (no need to move back/forward from/to pkg/shm). Parsing will 
> >>>> happen always once, as now cloning to shm in tm discards some parsed 
> >>>> headers, which may be needed in failure route or callbacks.
> >>>>     
> >>>>         
> >>> Havin a non-shm copy helps with locking and with moving cache lines
> >>> between cpus.
> >>>   
> >>>       
> >> as lot of processing is done with the message in shm anyhow, I tried to 
> >> figure out the impact of the rest of processing. So you say that is 
> >> better to copy to pkg and re-parse, than working with shm structures.
> >>
> >> The access is not sync'ed, as there will be same usage - in one process 
> >> - just the alloc/free will be different.
> >>     
> >
> > I think it would make sense to make the memory allocator used in parsers
> > somehow configurable, so that we can select whether the parsed structure is
> > stored in shared or in private memory.
> >
> > Not only this could save us some memory copying in modules like tm, but we
> > could also experiment with using the shared memory directly in some parts of
> > the code.
> >   
> Yes, being configurable will make happy everyone.

While I agree with making the parse function use a configurable memory
allocator, I don't agree with parsing in shm from the beginning.
That would be useful in an extremely limited number of scenarios (e.g.
 config containing only t_relay()).
In most configs there are different decisions or routing "branches" that
don't go through tm (e.g. if (...) drop;). Forcing shm allocation for
every packet would lead to a significant performance drop, especially on
machines with lots of cpu/cores (it scales terribly).
Apart from that there are the other problems I've mentioned.

The pkg_malloc() part takes an insignificant amount of time compared to
shm. Even with the memcpy() when cloning to shm we loose very little
performance when compared to going shm from the beginning for all the
packets (even for ones that won't be relayed/handled through tm).

We could have this discussion again once we have new shm memory
allocators (we have already some semi-experimental stuff, but it still
not on-par with fmalloc on single or dual cpus, much more work has to go
into them).
Until then and unless someone is able to show me some hard evidence (tests),
I won't agree with going shm from the beginning.

> 
> Since we are here, couple more thoughts about discussions we had in the 
> past on various mailing lists:
> - when there are changes in the parsed headers structure, there are 
> other parts of code forgotten to be updated, usually TM. I was thinking 
> it might worth to change the parsed field from void* to pointer to a 
> structure like:
> {
> void *data;
> void free_function(void *data);
> void *clone_function(void *date, malloc_function, free_function);
> }
> 
> Drawback is two more pointers, but if we remove most of "very unused" 
> direct header hooks, then we have same spare space...

It wastes too much space for no good reason. The forget to clone new
headers in tm problem is already handled by the compiler: a warning will
be generated if someone forgets to add a header to sip_msg_cloner.

Instead of the 2 function callbacks, we could add a "flags" member in which
we could mark if the header is alloc'ed in shm, pkg, should be freed a.s.o.
> 
> Advantage will be automatic and easy cloning/freeing.
> 

Andrei