diff mbox

[ovs-dev] ovs-thread: Fix memory leak in thread exit.

Message ID 1447192258-2214-1-git-send-email-blp@ovn.org
State Accepted
Headers show

Commit Message

Ben Pfaff Nov. 10, 2015, 9:50 p.m. UTC

Comments

Ben Pfaff Nov. 10, 2015, 9:54 p.m. UTC | #1 
There was supposed to be a commit message with this, but when I include
it the list refuses to post it.  Let's try part of it again, to narrow
it down:

    'n' is the number of keys, which are grouped into blocks of L2_SIZE
    indexes.  Even if only one key in a block is allocated, the whole block has
    a pointer to it that must be freed.  Thus, we need to round up instead of
    down.

On Tue, Nov 10, 2015 at 01:50:58PM -0800, Ben Pfaff wrote:
> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> index 88b92d1..2f6bc58 100644
> --- a/lib/ovs-thread.c
> +++ b/lib/ovs-thread.c
> @@ -665,7 +665,7 @@ ovsthread_key_destruct__(void *slots_)
>      n = n_keys;
>      ovs_mutex_unlock(&key_mutex);
>  
> -    for (i = 0; i < n / L2_SIZE; i++) {
> +    for (i = 0; i < DIV_ROUND_UP(n, L2_SIZE); i++) {
>          free(slots->p1[i]);
>      }
>      free(slots);
> -- 
> 2.1.3
>
Ben Pfaff Nov. 10, 2015, 9:57 p.m. UTC | #2 
OK, let's try adding one more line, does it get through?

    Signed-off-by: Ben Pfaff <blp@ovn.org>

On Tue, Nov 10, 2015 at 01:54:30PM -0800, Ben Pfaff wrote:
> There was supposed to be a commit message with this, but when I include
> it the list refuses to post it.  Let's try part of it again, to narrow
> it down:
> 
>     'n' is the number of keys, which are grouped into blocks of L2_SIZE
>     indexes.  Even if only one key in a block is allocated, the whole block has
>     a pointer to it that must be freed.  Thus, we need to round up instead of
>     down.
> 
> On Tue, Nov 10, 2015 at 01:50:58PM -0800, Ben Pfaff wrote:
> > diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> > index 88b92d1..2f6bc58 100644
> > --- a/lib/ovs-thread.c
> > +++ b/lib/ovs-thread.c
> > @@ -665,7 +665,7 @@ ovsthread_key_destruct__(void *slots_)
> >      n = n_keys;
> >      ovs_mutex_unlock(&key_mutex);
> >  
> > -    for (i = 0; i < n / L2_SIZE; i++) {
> > +    for (i = 0; i < DIV_ROUND_UP(n, L2_SIZE); i++) {
> >          free(slots->p1[i]);
> >      }
> >      free(slots);
> > -- 
> > 2.1.3
> >
Ben Pfaff Nov. 10, 2015, 9:59 p.m. UTC | #3 
Maybe email addresses are the problem?  I'll try a signed-off-by without
an "at" or angle brackets:

Signed-off-by: Ben Pfaff (blp at ovn.org)

On Tue, Nov 10, 2015 at 01:55:09PM -0800, Ben Pfaff wrote:
> OK, that got through, what about:
> 
>     Reported-at: https://github.com/openvswitch/ovs/pull/87
> 
> On Tue, Nov 10, 2015 at 01:54:30PM -0800, Ben Pfaff wrote:
> > There was supposed to be a commit message with this, but when I include
> > it the list refuses to post it.  Let's try part of it again, to narrow
> > it down:
> > 
> >     'n' is the number of keys, which are grouped into blocks of L2_SIZE
> >     indexes.  Even if only one key in a block is allocated, the whole block has
> >     a pointer to it that must be freed.  Thus, we need to round up instead of
> >     down.
> > 
> > On Tue, Nov 10, 2015 at 01:50:58PM -0800, Ben Pfaff wrote:
> > > diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> > > index 88b92d1..2f6bc58 100644
> > > --- a/lib/ovs-thread.c
> > > +++ b/lib/ovs-thread.c
> > > @@ -665,7 +665,7 @@ ovsthread_key_destruct__(void *slots_)
> > >      n = n_keys;
> > >      ovs_mutex_unlock(&key_mutex);
> > >  
> > > -    for (i = 0; i < n / L2_SIZE; i++) {
> > > +    for (i = 0; i < DIV_ROUND_UP(n, L2_SIZE); i++) {
> > >          free(slots->p1[i]);
> > >      }
> > >      free(slots);
> > > -- 
> > > 2.1.3
> > >
Jarno Rajahalme Nov. 10, 2015, 10:01 p.m. UTC | #4 
Let’s cut this off before it gets totally out of hand :-)

  Jarno

Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>

> On Nov 10, 2015, at 1:59 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> Maybe email addresses are the problem?  I'll try a signed-off-by without
> an "at" or angle brackets:
> 
> Signed-off-by: Ben Pfaff (blp at ovn.org)
> 
> On Tue, Nov 10, 2015 at 01:55:09PM -0800, Ben Pfaff wrote:
>> OK, that got through, what about:
>> 
>>    Reported-at: https://github.com/openvswitch/ovs/pull/87
>> 
>> On Tue, Nov 10, 2015 at 01:54:30PM -0800, Ben Pfaff wrote:
>>> There was supposed to be a commit message with this, but when I include
>>> it the list refuses to post it.  Let's try part of it again, to narrow
>>> it down:
>>> 
>>>    'n' is the number of keys, which are grouped into blocks of L2_SIZE
>>>    indexes.  Even if only one key in a block is allocated, the whole block has
>>>    a pointer to it that must be freed.  Thus, we need to round up instead of
>>>    down.
>>> 
>>> On Tue, Nov 10, 2015 at 01:50:58PM -0800, Ben Pfaff wrote:
>>>> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
>>>> index 88b92d1..2f6bc58 100644
>>>> --- a/lib/ovs-thread.c
>>>> +++ b/lib/ovs-thread.c
>>>> @@ -665,7 +665,7 @@ ovsthread_key_destruct__(void *slots_)
>>>>     n = n_keys;
>>>>     ovs_mutex_unlock(&key_mutex);
>>>> 
>>>> -    for (i = 0; i < n / L2_SIZE; i++) {
>>>> +    for (i = 0; i < DIV_ROUND_UP(n, L2_SIZE); i++) {
>>>>         free(slots->p1[i]);
>>>>     }
>>>>     free(slots);
>>>> -- 
>>>> 2.1.3
>>>> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Ben Pfaff Nov. 10, 2015, 10:05 p.m. UTC | #5 
I'd ordinarily say that, but this is the second patch I've been unable
to post to the list in a normal way.  They just disappear into the great
bit bucket in the mailing list somewhere and never appear.  If that
keeps happening it's going to cripple my development.

On Tue, Nov 10, 2015 at 02:01:09PM -0800, Jarno Rajahalme wrote:
> Let’s cut this off before it gets totally out of hand :-)
> 
>   Jarno
> 
> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
> 
> > On Nov 10, 2015, at 1:59 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > Maybe email addresses are the problem?  I'll try a signed-off-by without
> > an "at" or angle brackets:
> > 
> > Signed-off-by: Ben Pfaff (blp at ovn.org)
> > 
> > On Tue, Nov 10, 2015 at 01:55:09PM -0800, Ben Pfaff wrote:
> >> OK, that got through, what about:
> >> 
> >>    Reported-at: https://github.com/openvswitch/ovs/pull/87
> >> 
> >> On Tue, Nov 10, 2015 at 01:54:30PM -0800, Ben Pfaff wrote:
> >>> There was supposed to be a commit message with this, but when I include
> >>> it the list refuses to post it.  Let's try part of it again, to narrow
> >>> it down:
> >>> 
> >>>    'n' is the number of keys, which are grouped into blocks of L2_SIZE
> >>>    indexes.  Even if only one key in a block is allocated, the whole block has
> >>>    a pointer to it that must be freed.  Thus, we need to round up instead of
> >>>    down.
> >>> 
> >>> On Tue, Nov 10, 2015 at 01:50:58PM -0800, Ben Pfaff wrote:
> >>>> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> >>>> index 88b92d1..2f6bc58 100644
> >>>> --- a/lib/ovs-thread.c
> >>>> +++ b/lib/ovs-thread.c
> >>>> @@ -665,7 +665,7 @@ ovsthread_key_destruct__(void *slots_)
> >>>>     n = n_keys;
> >>>>     ovs_mutex_unlock(&key_mutex);
> >>>> 
> >>>> -    for (i = 0; i < n / L2_SIZE; i++) {
> >>>> +    for (i = 0; i < DIV_ROUND_UP(n, L2_SIZE); i++) {
> >>>>         free(slots->p1[i]);
> >>>>     }
> >>>>     free(slots);
> >>>> -- 
> >>>> 2.1.3
> >>>> 
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
Ben Pfaff Nov. 10, 2015, 10:11 p.m. UTC | #6 
OK, I give up.  There seems to be no rhyme or reason to why the list
drops my emails.

I'll push this, with the proper commit message and acks and whatever.

On Tue, Nov 10, 2015 at 02:05:17PM -0800, Ben Pfaff wrote:
> I'd ordinarily say that, but this is the second patch I've been unable
> to post to the list in a normal way.  They just disappear into the great
> bit bucket in the mailing list somewhere and never appear.  If that
> keeps happening it's going to cripple my development.
> 
> On Tue, Nov 10, 2015 at 02:01:09PM -0800, Jarno Rajahalme wrote:
> > Let’s cut this off before it gets totally out of hand :-)
> > 
> >   Jarno
> > 
> > Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
> > 
> > > On Nov 10, 2015, at 1:59 PM, Ben Pfaff <blp@ovn.org> wrote:
> > > 
> > > Maybe email addresses are the problem?  I'll try a signed-off-by without
> > > an "at" or angle brackets:
> > > 
> > > Signed-off-by: Ben Pfaff (blp at ovn.org)
> > > 
> > > On Tue, Nov 10, 2015 at 01:55:09PM -0800, Ben Pfaff wrote:
> > >> OK, that got through, what about:
> > >> 
> > >>    Reported-at: https://github.com/openvswitch/ovs/pull/87
> > >> 
> > >> On Tue, Nov 10, 2015 at 01:54:30PM -0800, Ben Pfaff wrote:
> > >>> There was supposed to be a commit message with this, but when I include
> > >>> it the list refuses to post it.  Let's try part of it again, to narrow
> > >>> it down:
> > >>> 
> > >>>    'n' is the number of keys, which are grouped into blocks of L2_SIZE
> > >>>    indexes.  Even if only one key in a block is allocated, the whole block has
> > >>>    a pointer to it that must be freed.  Thus, we need to round up instead of
> > >>>    down.
> > >>> 
> > >>> On Tue, Nov 10, 2015 at 01:50:58PM -0800, Ben Pfaff wrote:
> > >>>> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> > >>>> index 88b92d1..2f6bc58 100644
> > >>>> --- a/lib/ovs-thread.c
> > >>>> +++ b/lib/ovs-thread.c
> > >>>> @@ -665,7 +665,7 @@ ovsthread_key_destruct__(void *slots_)
> > >>>>     n = n_keys;
> > >>>>     ovs_mutex_unlock(&key_mutex);
> > >>>> 
> > >>>> -    for (i = 0; i < n / L2_SIZE; i++) {
> > >>>> +    for (i = 0; i < DIV_ROUND_UP(n, L2_SIZE); i++) {
> > >>>>         free(slots->p1[i]);
> > >>>>     }
> > >>>>     free(slots);
> > >>>> -- 
> > >>>> 2.1.3
> > >>>> 
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
> >
Justin Pettit Nov. 12, 2015, 1:26 a.m. UTC | #7 
> On Nov 10, 2015, at 2:11 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> OK, I give up.  There seems to be no rhyme or reason to why the list
> drops my emails.
> 
> I'll push this, with the proper commit message and acks and whatever.

Just for everyone else's benefit, we think we tracked the issue down to CudaMail (theoretically protecting us from spam). The issue should be resolved now. 

--Justin
diff mbox

Patch

diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index 88b92d1..2f6bc58 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -665,7 +665,7 @@  ovsthread_key_destruct__(void *slots_)
     n = n_keys;
     ovs_mutex_unlock(&key_mutex);
 
-    for (i = 0; i < n / L2_SIZE; i++) {
+    for (i = 0; i < DIV_ROUND_UP(n, L2_SIZE); i++) {
         free(slots->p1[i]);
     }
     free(slots);