You're right, it was slightly odd. Fixed in the next version.
Thanks for the review!
Ryan
On 5/29/14 1:17 PM, "Ethan Jackson" <et...@nicira.com> wrote:
>It's kinda weird that route_table_get_ifindex() is a globally
>accessible function which requires a mutex which is internal to the
>module. I think the answer is to remove it from the header file and
>make it static.
>
>Other than that looks fine.
>
>Acked-by: Ethan Jackson <et...@nicira.com>
>
>
>On Thu, May 29, 2014 at 12:31 PM, Ryan Wilson <wr...@nicira.com> wrote:
>> Due to patch fe83f8 (netdev: Remove netdev from global shash when
>> the user is changing interface configuration), netdevs can be
>> destructed and deallocated by revalidator and RCU threads. When
>> netdevs with class vport are destroyed, the routing table is
>> unregistered, possibly causing memory to be freed. This causes a
>> race condition with the main thread which calls route_table_run
>> periodically to check for routing table updates.
>>
>> This patch makes the route-table module thread-safe via a mutex.
>>
>> Bug #1258532
>> Signed-off-by: Ryan Wilson <wr...@nicira.com>
>> ---
>> lib/route-table.c | 19 +++++++++++++++++++
>> 1 file changed, 19 insertions(+)
>>
>> diff --git a/lib/route-table.c b/lib/route-table.c
>> index 2986d3d..19bd414 100644
>> --- a/lib/route-table.c
>> +++ b/lib/route-table.c
>> @@ -63,6 +63,7 @@ struct name_node {
>> char ifname[IFNAMSIZ]; /* Interface name. */
>> };
>>
>> +static struct ovs_mutex route_table_mutex = OVS_MUTEX_INITIALIZER;
>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>>
>> /* Global change number for route-table, which should be incremented
>> @@ -102,9 +103,12 @@ static struct name_node *name_node_lookup(int
>>ifi_index);
>> * Returns true if successful, otherwise false. */
>> bool
>> route_table_get_name(ovs_be32 ip, char name[IFNAMSIZ])
>> + OVS_EXCLUDED(route_table_mutex)
>> {
>> int ifindex;
>>
>> + ovs_mutex_lock(&route_table_mutex);
>> +
>> if (!name_table_valid) {
>> name_table_reset();
>> }
>> @@ -115,10 +119,12 @@ route_table_get_name(ovs_be32 ip, char
>>name[IFNAMSIZ])
>> nn = name_node_lookup(ifindex);
>> if (nn) {
>> ovs_strlcpy(name, nn->ifname, IFNAMSIZ);
>> + ovs_mutex_unlock(&route_table_mutex);
>> return true;
>> }
>> }
>>
>> + ovs_mutex_unlock(&route_table_mutex);
>> return false;
>> }
>>
>> @@ -130,6 +136,7 @@ route_table_get_name(ovs_be32 ip, char
>>name[IFNAMSIZ])
>> * Returns true if successful, otherwise false. */
>> bool
>> route_table_get_ifindex(ovs_be32 ip_, int *ifindex)
>> + OVS_REQUIRES(route_table_mutex)
>> {
>> struct route_node *rn;
>> uint32_t ip = ntohl(ip_);
>> @@ -168,7 +175,9 @@ route_table_get_change_seq(void)
>> * function before making any other route_table function calls. */
>> void
>> route_table_register(void)
>> + OVS_EXCLUDED(route_table_mutex)
>> {
>> + ovs_mutex_lock(&route_table_mutex);
>> if (!register_count) {
>> ovs_assert(!nln);
>> ovs_assert(!route_notifier);
>> @@ -186,6 +195,7 @@ route_table_register(void)
>> }
>>
>> register_count++;
>> + ovs_mutex_unlock(&route_table_mutex);
>> }
>>
>> /* Users of the route_table module should unregister themselves with
>>this
>> @@ -193,7 +203,9 @@ route_table_register(void)
>> * calls. */
>> void
>> route_table_unregister(void)
>> + OVS_EXCLUDED(route_table_mutex)
>> {
>> + ovs_mutex_lock(&route_table_mutex);
>> register_count--;
>>
>> if (!register_count) {
>> @@ -206,12 +218,15 @@ route_table_unregister(void)
>> hmap_destroy(&route_map);
>> name_table_uninit();
>> }
>> + ovs_mutex_unlock(&route_table_mutex);
>> }
>>
>> /* Run periodically to update the locally maintained routing table. */
>> void
>> route_table_run(void)
>> + OVS_EXCLUDED(route_table_mutex)
>> {
>> + ovs_mutex_lock(&route_table_mutex);
>> if (nln) {
>> rtnetlink_link_run();
>> nln_run(nln);
>> @@ -220,16 +235,20 @@ route_table_run(void)
>> route_table_reset();
>> }
>> }
>> + ovs_mutex_unlock(&route_table_mutex);
>> }
>>
>> /* Causes poll_block() to wake up when route_table updates are
>>required. */
>> void
>> route_table_wait(void)
>> + OVS_EXCLUDED(route_table_mutex)
>> {
>> + ovs_mutex_lock(&route_table_mutex);
>> if (nln) {
>> rtnetlink_link_wait();
>> nln_wait(nln);
>> }
>> + ovs_mutex_unlock(&route_table_mutex);
>> }
>>
>> static int
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>>
>>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman
>>/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=TfBS78Vw3dzttvXidhbff
>>g%3D%3D%0A&m=HtX0nWMlpz1Cbq73iOx4PEPkgfnDgJjdIM6pPXRKRJo%3D%0A&s=5f977985
>>6bffd4b35554765a1a1ec3f3fc62dd3c4db7461b6730002ec97b38e6
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
