[ovs-dev] [PATCH v2] lib/dpif-netdev: Integrate megaflow classifier.

Alex Wang alexw at nicira.com
Tue Oct 14 23:22:36 UTC 2014


>
> @ -2697,19 +2845,28 @@ fast_path_processing(struct dp_netdev_pmd_thread
>> *pmd,
>>                  ? &put_actions
>>                  : &actions;
>>
>> -            ovs_mutex_lock(&dp->flow_mutex);
>> -            /* XXX: There's a brief race where this flow could have
>> already
>> -             * been installed since we last did the flow lookup.  This
>> could be
>> -             * solved by moving the mutex lock outside the loop, but
>> that's an
>> -             * awful long time to be locking everyone out of making flow
>> -             * installs.  If we move to a per-core classifier, it would
>> be
>> -             * reasonable. */
>> -            if (OVS_LIKELY(error != ENOSPC)
>> -                && !dp_netdev_lookup_flow(dp, mfs[i])) {
>> -                dp_netdev_flow_add(dp, &match, ofpbuf_data(add_actions),
>> -                                   ofpbuf_size(add_actions));
>> +            if (OVS_LIKELY(error != ENOSPC)) {
>> +                keys[i].hash = netdev_flow_key_hash(&keys[i]);
>> +
>> +                /* XXX: There's a race window where a flow covering this
>> packet
>> +                 * could have already been installed since we last did
>> the flow
>> +                 * lookup before upcall.  This could be solved by moving
>> the
>> +                 * mutex lock outside the loop, but that's an awful long
>> time
>> +                 * to be locking everyone out of making flow installs.
>> If we
>> +                 * move to a per-core classifier, it would be
>> reasonable. */
>> +                ovs_mutex_lock(&dp->flow_mutex);
>> +                netdev_flow = dp_netdev_lookup_flow(dp, &keys[i]);
>> +                if (OVS_LIKELY(!netdev_flow)) {
>> +                    netdev_flow = dp_netdev_flow_add(dp, &match,
>> +
>>  ofpbuf_data(add_actions),
>> +
>>  ofpbuf_size(add_actions));
>> +                }
>> +                ovs_mutex_unlock(&dp->flow_mutex);
>> +
>> +                /* EMC uses different hash. */
>> +                keys[i].hash = dpif_packet_get_dp_hash(packets[i]);
>> +                emc_insert(flow_cache, &keys[i], netdev_flow);
>>
>
>
> Could we just assign the 'rules[i] = &netdev_flow->cr;' and install them
> in the
> end of function.
>
> Also seems to me that if we do not do 'rules[i] = &netdev_flow->cr;', the
> pkt
> will not be queued in 'dp_netdev_queue_batches()' at the end of function.
>


Sorry, I missed this just above the modified code:

            /* We can't allow the packet batching in the next loop to
execute
             * the actions.  Otherwise, if there are any slow path actions,
             * we'll send the packet up twice. */
            dp_netdev_execute_actions(pmd, &packets[i], 1, true,
                                      ofpbuf_data(&actions),
                                      ofpbuf_size(&actions));


So, I'm fine with this change.




Besides, here is another issue:

the output of function 'netdev_flow_key_hash()' seems not got used,
for dpcls_lookup(), we calculate the hash inside the function,
for emc_lookup(), we use 'dpif_netdev_packet_get_dp_hash()'
for flow_table(), we use the 'flow_hash(&flow->flow, 0)'

Could you take a look?

Thanks,
Alex Wang,



More information about the dev mailing list