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).
> @@ -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?
> @@ -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.
> @@ -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.
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.
> 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
