On Mon, Feb 6, 2012 at 3:25 PM, Jesse Gross <[email protected]> wrote:
> On Thu, Feb 2, 2012 at 6:58 AM, Pravin B Shelar <[email protected]> wrote:
>> diff --git a/datapath/linux/compat/flex_array.c
>> b/datapath/linux/compat/flex_array.c
>> index f2d0cd2..6621bce 100644
>> --- a/datapath/linux/compat/flex_array.c
>> +++ b/datapath/linux/compat/flex_array.c
>> @@ -153,7 +153,7 @@ void flex_array_free_parts(const struct flex_array *fa)
>> if (elements_fit_in_base(fa))
>> return;
>> for (part_nr = 0; part_nr < FLEX_ARRAY_NR_BASE_PTRS; part_nr++)
>> - kfree(fa->parts[part_nr]);
>> + kfree(rcu_dereference_raw(fa->parts[part_nr]));
>
> I think in this case it's better to just cast away the __rcu
> annotation since at this point it's not really being protected by RCU
> (the caller must have exclusive access).
>
ok.
>> @@ -183,7 +183,7 @@ __fa_get_part(struct flex_array *fa, int part_nr, gfp_t
>> flags)
>> if (!(flags & __GFP_ZERO))
>> memset(part, 0, sizeof(struct flex_array_part));
>
> This got changed in the previous patch but I don't understand it. Why
> do we memset the block of memory to zero if the called didn't pass
> __GFP_ZERO?
right, it is not required.
>
>> @@ -225,6 +225,7 @@ int flex_array_put(struct flex_array *fa, unsigned int
>> element_nr, void *src,
>> dst = &part->elements[index_inside_part(fa, element_nr, part_nr)];
>> + smp_wmb();
>> memcpy(dst, src, fa->element_size);
>
> This memory barrier doesn't look right to me. It's guaranteeing that
> other CPUs will see the pointer written before the data that is being
> pointed to but that's the opposite of what we want. This is a good
> example of why I wanted to only use the RCU primitives as opposed to
> open-coding as it is difficult to get this stuff right.
>
I wanted to keep RCU like API for flex_array-get, set APIs. Thats why
I used write barrier.
I am not sure why is it opposite. I think smp_wmb() will make sure all
data is flushed to memory before it update flex array. In our case
members of dp would get written before dp is set in flex array. It is
same as rcu_assign_pointer().
I can not use RCU API here as its not pointer.
>> @@ -329,11 +331,12 @@ void *flex_array_get(const struct flex_array *fa,
>> unsigned int element_nr)
>> return NULL;
>> if (element_nr >= fa->total_nr_elements)
>> return NULL;
>> - if (elements_fit_in_base(fa))
>> + if (elements_fit_in_base(fa)) {
>> + smp_read_barrier_depends();
>
> It's not clear to me what ordering constraint the
> smp_read_barrier_depends() is enforcing. The goal seems to be that fa
> was written after the element but that's doesn't make sense.
>
its for smp_wmb() above.
> I think the biggest problem here is that there's no clear
> documentation on what guarantees we're providing. There's two
> possibilities that I see: that the flex array data structure itself is
> safe to read as it is being expanded on a different CPU or that both
> the data structure and the data being written are safe. For our
> purposes we only care about the former because the data are pointers
> and we can initialize it to zero so everything is atomic. The latter
> is impossible to enforce for arbitrary data with the way that flex
> arrays are set up because after a part is allocated there's no pointer
> write to create an atomic switch with which we can enforce ordering.
> It looks like the RCU primitives that you used protect the structure
> itself and the open coded components are trying to protect the data.
>
ok, It will not work when non pointer data is stored in flex_array. I
will move mem barrier to caller.
>> diff --git a/datapath/linux/compat/include/linux/flex_array.h
>> b/datapath/linux/compat/include/linux/flex_array.h
>> index 6fdaef7..334dc46 100644
>> --- a/datapath/linux/compat/include/linux/flex_array.h
>> +++ b/datapath/linux/compat/include/linux/flex_array.h
>> @@ -73,6 +73,8 @@ void flex_array_free(const struct flex_array *fa);
>> void flex_array_free_parts(const struct flex_array *fa);
>> int flex_array_put(struct flex_array *fa, unsigned int element_nr, void
>> *src,
>> gfp_t flags);
>> +
>> +#define flex_array_clear rpl_flex_array_clear
>> int flex_array_clear(struct flex_array *fa, unsigned int element_nr);
>
> There won't be any duplication of symbols because the original
> flex_array.h is not included at all. It's not good to do this ad-hoc
> replacement of functions because if there was a potential for conflict
> there would be bigger problems as the actual structure is different on
> older kernels.
ok.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
