Relax metadata FDB api, and handle fdb notifications in metadata (son…

…ic-net#128) * Relax metadata FDB api, and handle fdb notifications in metadata * Add workaround for missing fdb event type in notification
dmytroxshevchuk · Nov 18, 2016 · b60f4bb · b60f4bb
1 parent ac3ede5
commit b60f4bb
Show file tree

Hide file tree

Showing 11 changed files with 133 additions and 31 deletions.
diff --git a/lib/inc/sai_redis.h b/lib/inc/sai_redis.h
@@ -48,7 +48,6 @@ extern swss::Table *g_ridToVid;
 
 extern swss::RedisClient               *g_redisClient;
 
-extern std::mutex g_mutex;
 extern std::mutex g_apimutex;
 
 extern const sai_acl_api_t              redis_acl_api;

diff --git a/lib/src/sai_redis_generic_create.cpp b/lib/src/sai_redis_generic_create.cpp
@@ -39,8 +39,6 @@ sai_status_t internal_redis_generic_create(
         _In_ uint32_t attr_count,
         _In_ const sai_attribute_t *attr_list)
 {
-    std::lock_guard<std::mutex> lock(g_mutex);
-
     SWSS_LOG_ENTER();
 
     if (attr_count > 0 && attr_list == NULL)

diff --git a/lib/src/sai_redis_generic_get.cpp b/lib/src/sai_redis_generic_get.cpp
@@ -45,8 +45,6 @@ sai_status_t internal_redis_generic_get(
         _In_ uint32_t attr_count,
         _Out_ sai_attribute_t *attr_list)
 {
-    std::lock_guard<std::mutex> lock(g_mutex);
-
     SWSS_LOG_ENTER();
 
     std::vector<swss::FieldValueTuple> entry = SaiAttributeList::serialize_attr_list(

diff --git a/lib/src/sai_redis_generic_remove.cpp b/lib/src/sai_redis_generic_remove.cpp
@@ -6,8 +6,6 @@ sai_status_t internal_redis_generic_remove(
         _In_ sai_object_type_t object_type,
         _In_ const std::string &serialized_object_id)
 {
-    std::lock_guard<std::mutex> lock(g_mutex);
-
     SWSS_LOG_ENTER();
 
     std::string str_object_type = sai_serialize_object_type(object_type);

diff --git a/lib/src/sai_redis_generic_set.cpp b/lib/src/sai_redis_generic_set.cpp
@@ -7,8 +7,6 @@ sai_status_t internal_redis_generic_set(
         _In_ const std::string &serialized_object_id,
         _In_ const sai_attribute_t *attr)
 {
-    std::lock_guard<std::mutex> lock(g_mutex);
-
     SWSS_LOG_ENTER();
 
     std::vector<swss::FieldValueTuple> entry = SaiAttributeList::serialize_attr_list(

diff --git a/lib/src/sai_redis_interfacequery.cpp b/lib/src/sai_redis_interfacequery.cpp
@@ -1,7 +1,6 @@
 #include <string.h>
 #include "sai_redis.h"
 
-std::mutex g_mutex;
 std::mutex g_apimutex;
 
 service_method_table_t g_services;
@@ -21,7 +20,7 @@ sai_status_t sai_api_initialize(
         _In_ uint64_t flags,
         _In_ const service_method_table_t* services)
 {
-    std::lock_guard<std::mutex> lock(g_mutex);
+    std::lock_guard<std::mutex> lock(g_apimutex);
 
     SWSS_LOG_ENTER();
 
@@ -78,7 +77,7 @@ sai_status_t sai_log_set(
         _In_ sai_api_t sai_api_id,
         _In_ sai_log_level_t log_level)
 {
-    std::lock_guard<std::mutex> lock(g_mutex);
+    std::lock_guard<std::mutex> lock(g_apimutex);
 
     SWSS_LOG_ENTER();
 
@@ -169,7 +168,7 @@ sai_status_t sai_api_query(
         _In_ sai_api_t sai_api_id,
         _Out_ void** api_method_table)
 {
-    std::lock_guard<std::mutex> lock(g_mutex);
+    std::lock_guard<std::mutex> lock(g_apimutex);
 
     SWSS_LOG_ENTER();
 
@@ -298,7 +297,7 @@ sai_status_t sai_api_query(
 
 sai_status_t sai_api_uninitialize(void)
 {
-    std::lock_guard<std::mutex> lock(g_mutex);
+    std::lock_guard<std::mutex> lock(g_apimutex);
 
     SWSS_LOG_ENTER();
 

diff --git a/lib/src/sai_redis_notifications.cpp b/lib/src/sai_redis_notifications.cpp
@@ -33,6 +33,16 @@ void handle_fdb_event(
 
     sai_deserialize_fdb_event_ntf(data, count, &fdbevent);
 
+    {
+        std::lock_guard<std::mutex> lock(g_apimutex);
+
+        // NOTE: this meta api must be under mutex since
+        // it will access meta DB and notification comes
+        // from different thread
+
+        meta_sai_on_fdb_event(count, fdbevent);
+    }
+
     auto on_fdb_event = redis_switch_notifications.on_fdb_event;
 
     if (on_fdb_event != NULL)

diff --git a/lib/src/sai_redis_switch.cpp b/lib/src/sai_redis_switch.cpp
@@ -94,8 +94,6 @@ sai_status_t redis_initialize_switch(
 {
     std::lock_guard<std::mutex> apilock(g_apimutex);
 
-    std::lock_guard<std::mutex> lock(g_mutex);
-
     SWSS_LOG_ENTER();
 
     if (g_switchInitialized)
@@ -151,8 +149,6 @@ void redis_shutdown_switch(
 {
     std::lock_guard<std::mutex> apilock(g_apimutex);
 
-    std::lock_guard<std::mutex> lock(g_mutex);
-
     SWSS_LOG_ENTER();
 
     if (!g_switchInitialized)
@@ -195,8 +191,6 @@ sai_status_t redis_connect_switch(
 {
     std::lock_guard<std::mutex> apilock(g_apimutex);
 
-    std::lock_guard<std::mutex> lock(g_mutex);
-
     SWSS_LOG_ENTER();
 
     SWSS_LOG_ERROR("not implemented");
@@ -217,8 +211,6 @@ void redis_disconnect_switch(void)
 {
     std::lock_guard<std::mutex> apilock(g_apimutex);
 
-    std::lock_guard<std::mutex> lock(g_mutex);
-
     SWSS_LOG_ENTER();
 
     SWSS_LOG_ERROR("not implemented");

diff --git a/meta/sai_meta.cpp b/meta/sai_meta.cpp
@@ -736,7 +736,7 @@ void set_object(
     if (!object_exists(key))
     {
         SWSS_LOG_ERROR("FATAL: object %s don't exists", key.c_str());
-        throw;
+        throw std::runtime_error("FATAL: object don't exists" + key);
     }
 
     META_LOG_DEBUG(md, "set attribute %d on %s", attr->id, key.c_str());
@@ -2361,7 +2361,8 @@ void meta_generic_validation_post_create(
             break;
 
         case SAI_OBJECT_TYPE_FDB:
-            vlan_reference_inc(meta_key.key.fdb_entry.vlan_id);
+            // NOTE: we don't increase vlan reference on FDB entries, ignored
+            // vlan_reference_inc(meta_key.key.fdb_entry.vlan_id);
             break;
 
         case SAI_OBJECT_TYPE_VLAN:
@@ -2679,7 +2680,8 @@ void meta_generic_validation_post_remove(
             break;
 
         case SAI_OBJECT_TYPE_FDB:
-            vlan_reference_dec(meta_key.key.fdb_entry.vlan_id);
+            // NOTE: we don't decrease vlan reference on FDB entries, ignored
+            // vlan_reference_dec(meta_key.key.fdb_entry.vlan_id);
             break;
 
         case SAI_OBJECT_TYPE_TRAP:
@@ -3321,7 +3323,8 @@ sai_status_t meta_sai_get_switch(
 
 sai_status_t meta_sai_validate_fdb_entry(
         _In_ const sai_fdb_entry_t* fdb_entry,
-        _In_ bool create)
+        _In_ bool create,
+        _In_ bool get = false)
 {
     SWSS_LOG_ENTER();
 
@@ -3342,7 +3345,9 @@ sai_status_t meta_sai_validate_fdb_entry(
     }
 
     // check if vlan exists
+    // NOTE: this is disabled on purpose, we can create/set/get/remove fdb entris with non existing vlans
 
+    /*
     sai_object_meta_key_t meta_key_vlan = { .object_type = SAI_OBJECT_TYPE_VLAN, .key = { .vlan_id = vlan_id } };
 
     std::string key_vlan = get_object_meta_key_string(meta_key_vlan);
@@ -3353,6 +3358,7 @@ sai_status_t meta_sai_validate_fdb_entry(
 
         return SAI_STATUS_INVALID_PARAMETER;
     }
+    */
 
     // check if fdb entry exists
 
@@ -3374,7 +3380,7 @@ sai_status_t meta_sai_validate_fdb_entry(
 
     // set, get, remove
 
-    if (!object_exists(key_fdb))
+    if (!object_exists(key_fdb) && !get)
     {
         SWSS_LOG_ERROR("object key %s don't exists", key_fdb.c_str());
 
@@ -3541,7 +3547,9 @@ sai_status_t meta_sai_get_fdb_entry(
 {
     SWSS_LOG_ENTER();
 
-    sai_status_t status = meta_sai_validate_fdb_entry(fdb_entry, false);
+    // NOTE: when doing get, entry may not exist on metadata db
+
+    sai_status_t status = meta_sai_validate_fdb_entry(fdb_entry, false, true);
 
     if (status != SAI_STATUS_SUCCESS)
     {
@@ -4838,3 +4846,99 @@ sai_status_t meta_sai_get_oid(
 
     return status;
 }
+
+// NOTIFICATIONS
+
+void meta_sai_on_fdb_event_single(
+        _In_ const sai_fdb_event_notification_data_t& data)
+{
+    SWSS_LOG_ENTER();
+
+    // NOTE: vlan may not exists
+
+    sai_object_meta_key_t meta_key_vlan = { .object_type = SAI_OBJECT_TYPE_VLAN, .key = { .vlan_id = data.fdb_entry.vlan_id} };
+
+    std::string key_vlan = get_object_meta_key_string(meta_key_vlan);
+
+    if (!object_exists(key_vlan))
+    {
+        SWSS_LOG_WARN("object key %s don't exists", key_vlan.c_str());
+    }
+
+    const sai_object_meta_key_t meta_key_fdb = { .object_type = SAI_OBJECT_TYPE_FDB, .key = { .fdb_entry = data.fdb_entry } };
+
+    std::string key_fdb = get_object_meta_key_string(meta_key_fdb);
+
+    switch (data.event_type)
+    {
+        case SAI_FDB_EVENT_LEARNED:
+
+            if (object_exists(key_fdb))
+            {
+                SWSS_LOG_WARN("object key %s alearedy exists, but received LEARNED event", key_fdb.c_str());
+                break;
+            }
+
+            {
+                sai_attribute_t *list = data.attr;
+                uint32_t count = data.attr_count;
+
+                sai_attribute_t local[2]; // 2 for port id and type
+
+                if (count == 1)
+                {
+                    // workaround for missing "TYPE" attribute on notification
+
+                    local[0] = data.attr[0]; // copy 1st attr
+                    local[1].id = SAI_FDB_ENTRY_ATTR_TYPE;
+                    local[1].value.s32 = SAI_FDB_ENTRY_DYNAMIC; // assume learned entries are always dynamic
+
+                    list = local;
+                    count = 2; // now we added type
+                }
+
+                sai_status_t status = meta_generic_validation_create(meta_key_fdb, count, list);
+
+                if (status == SAI_STATUS_SUCCESS)
+                {
+                    meta_generic_validation_post_create(meta_key_fdb, count, list);
+                }
+                else
+                {
+                    SWSS_LOG_ERROR("failed to insert %s received in notification: %s", key_fdb.c_str(), sai_serialize_status(status).c_str());
+                }
+            }
+
+            break;
+
+        case SAI_FDB_EVENT_AGED:
+        case SAI_FDB_EVENT_FLUSHED:
+
+            if (!object_exists(key_fdb))
+            {
+                SWSS_LOG_WARN("object key %s don't exist but received AGED/FLUSHED event", key_fdb.c_str());
+                break;
+            }
+
+            meta_generic_validation_post_remove(meta_key_fdb);
+
+            break;
+
+        default:
+
+            SWSS_LOG_ERROR("got FDB_ENTRY notification with unknown event_type %d, bug?", data.event_type);
+            break;
+    }
+}
+
+void meta_sai_on_fdb_event(
+        _In_ uint32_t count,
+        _In_ sai_fdb_event_notification_data_t *data)
+{
+    SWSS_LOG_ENTER();
+
+    for (uint32_t i = 0; i < count; ++i)
+    {
+        meta_sai_on_fdb_event_single(data[i]);
+    }
+}
diff --git a/meta/sai_meta.h b/meta/sai_meta.h
@@ -918,4 +918,10 @@ extern sai_status_t meta_sai_get_route_entry(
         _Inout_ sai_attribute_t *attr_list,
         _In_ sai_get_route_attribute_fn get);
 
+// NOTIFICATIONS
+
+extern void meta_sai_on_fdb_event(
+        _In_ uint32_t count,
+        _In_ sai_fdb_event_notification_data_t *data);
+
 #endif // __SAI_META_H__
diff --git a/meta/sai_meta_fdb.cpp b/meta/sai_meta_fdb.cpp
@@ -88,11 +88,11 @@ const sai_attr_metadata_t sai_fdb_attr_metadata[] = {
         .attrid                 = SAI_FDB_ENTRY_ATTR_PACKET_ACTION,
         .attridname             = "SAI_FDB_ENTRY_ATTR_PACKET_ACTION",
         .serializationtype      = SAI_SERIALIZATION_TYPE_INT32,
-        .flags                  = SAI_ATTR_FLAGS_MANDATORY_ON_CREATE | SAI_ATTR_FLAGS_CREATE_ONLY,
+        .flags                  = SAI_ATTR_FLAGS_CREATE_AND_SET,
         .allowedobjecttypes     = { },
         .allownullobjectid      = false,
-        .defaultvaluetype       = SAI_DEFAULT_VALUE_TYPE_NONE,
-        .defaultvalue           = { },
+        .defaultvaluetype       = SAI_DEFAULT_VALUE_TYPE_CONST,
+        .defaultvalue           = { .s32 = SAI_PACKET_ACTION_FORWARD },
         .enumtypestr            = StringifyEnum ( sai_packet_action_t ),
         .enumallowedvalues      = ENUM_VALUES ( sai_packet_action_t ),
         .enummetadata           = &metadata_enum_sai_packet_action_t,