Re: MBUF behaviour

[Christopher Collins]

On Sat, Apr 07, 2018 at 08:15:55PM +0530, Aditya Xavier wrote:
> That explains everything. However, one question. 
> 
> When we do a os_mbuf_free_chain, shouldn’t om_data also provide “”, instead 
> of the previous value ?

The contents of an mbuf's data buffer beyond `om_len` are always
indeterminate.  The issue here is that the app prints the mbuf data
despite `om_len` being equal to 0.

What is actually happening is `os_mbuf_get()` is returning the same mbuf
that was just freed.  This is a consequence of Mynewt's mempool
implementation; elements are allocated in the reverse order they were
freed.  Since the app only allocates one mbuf at a time, the mempool
always returns a pointer to the same mbuf.

The mbufpool implementation could zero out an mbuf when it gets
allocated.  This would prevent the app from printing stale contents.
However, this behavior would be a waste of time; setting `om_len` to 0
is sufficient as long as the app respects the mbuf API.

Chris

> 
> I understand its not wise to delete the data from the memory for the sake of 
> efficiency, however was wondering if thats the expected result.
> 
> Thanks for the explanation though. Would change the code accordingly.
> 
> Thanks,
> Aditya Xavier.
> 
> 
> > On 07-Apr-2018, at 8:55 AM, Christopher Collins  wrote:
> > 
> > Hi Aditya,
> > 
> > On Sat, Apr 07, 2018 at 07:59:44AM +0530, Aditya Xavier wrote:
> >> Hi Christopher,
> >> 
> >> That is the expected behaviour, however if you try running the sample app 
> >> I gave you would notice the following 
> >> After step 11, I.e initialise the os_mbuf by doing a os_mbuf_get from a 
> >> mempool, the new value is overwritten on the previous value.
> >> 
> >> I.e
> >> 1. Accessing the mbuf after doing a free chain, om_data still holds the 
> >> value.
> >> 2. Initialising it again by doing a os_mbuf_get is not getting me a clean 
> >> mbuf, rather it holds the previous value and om_mbuf_copyinto merely 
> >> overwrites it. So incase the new string length is smaller, om_data would 
> >> return wrong value.
> >> 
> >> Am sorry if am not able to explain it properly however I would appreciate 
> >> it if you can test the app once.
> > 
> > I ran your app, and I see nothing unusual in the output.  Here is what I
> > get:
> > 
> >(gdb) r
> >Starting program:
> >
> > /mnt/data2/work/micosa/repos/mynewt-core/bin/targets/blinky-sim/app/apps/blinky/blinky.elf
> >uart0 at /dev/pts/16
> >UART MBUF Created 1 to 1
> >Received Value :- abc
> >Received Length :- 3
> >Value after Reinit :- abc
> >Length after Reinit :- 0
> >Received Value :- hello
> >Received Length :- 5
> >Value after Reinit :- hello
> >Length after Reinit :- 0
> >Received Value :- gagao
> >Received Length :- 4
> >Value after Reinit :- gagao
> >Length after Reinit :- 0
> > 
> > To get this output, I typed the following strings into the console:
> > 
> >abc
> >hello
> >gaga
> > 
> > If I understand correctly, your concern is the following part of the
> > output:
> > 
> >Received Value :- gagao
> >Received Length :- 4
> >Value after Reinit :- gagao
> >Length after Reinit :- 0
> > 
> > Specifically, you are unsure why:
> > 
> >* The first line contains "gagao" rather than "gaga".
> >* The third line contains "gagao" rather than "".
> > 
> > Your program uses the `%s` format specifier to print the contents of an
> > mbuf.  This is probably not what you want, for a number of reasons:
> > 
> >* Mbuf contents are not typically null-terminated.
> >* Mbuf contents are not guaranteed to be contiguous (i.e., multiple
> >  bufers may be chained).
> > 
> > Here is a reliable, if inefficient, way to print an mbuf as a string:
> > 
> >static void
> >print_mbuf(const struct os_mbuf *om)
> >{
> >int i;
> > 
> >for (; om != NULL; om = SLIST_NEXT(om, om_next)) {
> >for (i = 0; i < om->om_len; i++) {
> >putchar(om->om_data[i]);
> >}
> >}
> >}
> > 
> > If you are sure the mbuf is not chained, then you don't need the outer
> > loop.
> > 
> > Chris
> 

Re: MBUF behaviour

[Christopher Collins]

Hi Aditya,

On Sat, Apr 07, 2018 at 07:59:44AM +0530, Aditya Xavier wrote:
> Hi Christopher,
> 
> That is the expected behaviour, however if you try running the sample app I 
> gave you would notice the following 
> After step 11, I.e initialise the os_mbuf by doing a os_mbuf_get from a 
> mempool, the new value is overwritten on the previous value.
> 
> I.e
> 1. Accessing the mbuf after doing a free chain, om_data still holds the value.
> 2. Initialising it again by doing a os_mbuf_get is not getting me a clean 
> mbuf, rather it holds the previous value and om_mbuf_copyinto merely 
> overwrites it. So incase the new string length is smaller, om_data would 
> return wrong value.
> 
> Am sorry if am not able to explain it properly however I would appreciate it 
> if you can test the app once.

I ran your app, and I see nothing unusual in the output.  Here is what I
get:

(gdb) r
Starting program:

/mnt/data2/work/micosa/repos/mynewt-core/bin/targets/blinky-sim/app/apps/blinky/blinky.elf
uart0 at /dev/pts/16
UART MBUF Created 1 to 1
Received Value :- abc
Received Length :- 3
Value after Reinit :- abc
Length after Reinit :- 0
Received Value :- hello
Received Length :- 5
Value after Reinit :- hello
Length after Reinit :- 0
Received Value :- gagao
Received Length :- 4
Value after Reinit :- gagao
Length after Reinit :- 0

To get this output, I typed the following strings into the console:

abc
hello
gaga

If I understand correctly, your concern is the following part of the
output:

Received Value :- gagao
Received Length :- 4
Value after Reinit :- gagao
Length after Reinit :- 0

Specifically, you are unsure why:

* The first line contains "gagao" rather than "gaga".
* The third line contains "gagao" rather than "".

Your program uses the `%s` format specifier to print the contents of an
mbuf.  This is probably not what you want, for a number of reasons:

* Mbuf contents are not typically null-terminated.
* Mbuf contents are not guaranteed to be contiguous (i.e., multiple
  bufers may be chained).

Here is a reliable, if inefficient, way to print an mbuf as a string:

static void
print_mbuf(const struct os_mbuf *om)
{
int i;

for (; om != NULL; om = SLIST_NEXT(om, om_next)) {
for (i = 0; i < om->om_len; i++) {
putchar(om->om_data[i]);
}
}
}

If you are sure the mbuf is not chained, then you don't need the outer
loop.

Chris

Re: MBUF behaviour

[Aditya Xavier]

Hi Christopher,

That is the expected behaviour, however if you try running the sample app I 
gave you would notice the following 
After step 11, I.e initialise the os_mbuf by doing a os_mbuf_get from a 
mempool, the new value is overwritten on the previous value.

I.e
1. Accessing the mbuf after doing a free chain, om_data still holds the value.
2. Initialising it again by doing a os_mbuf_get is not getting me a clean mbuf, 
rather it holds the previous value and om_mbuf_copyinto merely overwrites it. 
So incase the new string length is smaller, om_data would return wrong value.

Am sorry if am not able to explain it properly however I would appreciate it if 
you can test the app once.

Thanks,
Aditya Xavier 

> On 07-Apr-2018, at 7:32 AM, Christopher Collins  wrote:
> 
> Hi Aditya,
> 
>> On Fri, Apr 06, 2018 at 07:36:41PM +0530, Aditya Xavier wrote:
>> Hi Mynewt Team,
>> 
>> Please help me understand the behavior of MBUF.
>> 
>> PFB the steps I did :-
>> 
>> 1.os_mempool_init
>> 2.os_mbuf_pool_init
>> 3.Initialized a os_mbuf by os_mbuf_get
>> 4.Triggered Console Reader.
>> 5.os_mbuf_copyinto the console_buf into the os_mbuf
>> 6.Did a eventq_put to get into a event callback.
>> 7.Read os_mbuf value & os_mbuf len
>> 8.os_mbuf_free_chain
>> 9.Read os_mbuf value & os_mbuf len
>> 10.Initialized a os_mbuf by os_mbuf_get
>> 11.Repeat step 4 onwards.
>> 
>> Problem :-
>> In step 7, I read the previous string, however the length is correct.
>> In step 9, I read the previous string, however the length is 0.
> 
> `os_mbuf_free_chain` frees the mbuf chain back to its source pool.  From
> that point, accessing the mbuf via this pointer is an error.
> 
> This is analogous to the following example:
> 
>int *x = malloc(sizeof(*x));
>*x = 99;
>free(x);
>printf("*x = %d\n", *x);
> 
> In other words, don't access something after you free it! :)  You'll
> need to allocate a new mbuf if you need one after freeing the first.
> 
> Chris

Re: MBUF behaviour

[Christopher Collins]

Hi Aditya,

On Fri, Apr 06, 2018 at 07:36:41PM +0530, Aditya Xavier wrote:
> Hi Mynewt Team,
> 
> Please help me understand the behavior of MBUF.
> 
> PFB the steps I did :-
> 
> 1.os_mempool_init
> 2.os_mbuf_pool_init
> 3.Initialized a os_mbuf by os_mbuf_get
> 4.Triggered Console Reader.
> 5.os_mbuf_copyinto the console_buf into the os_mbuf
> 6.Did a eventq_put to get into a event callback.
> 7.Read os_mbuf value & os_mbuf len
> 8.os_mbuf_free_chain
> 9.Read os_mbuf value & os_mbuf len
> 10.   Initialized a os_mbuf by os_mbuf_get
> 11.   Repeat step 4 onwards.
> 
> Problem :-
> In step 7, I read the previous string, however the length is correct.
> In step 9, I read the previous string, however the length is 0.

`os_mbuf_free_chain` frees the mbuf chain back to its source pool.  From
that point, accessing the mbuf via this pointer is an error.

This is analogous to the following example:

int *x = malloc(sizeof(*x));
*x = 99;
free(x);
printf("*x = %d\n", *x);

In other words, don't access something after you free it! :)  You'll
need to allocate a new mbuf if you need one after freeing the first.

Chris