On Fri, Dec 19, 2014 at 10:26 AM, Joe Stringer <joestrin...@nicira.com> wrote:
> On 18 December 2014 at 17:16, Andy Zhou <az...@nicira.com> wrote:
>> I don't strongly object the current version either (with the fix
>> above), but this version seems simpler.
>
> OK, but this is tangential to the original patch so I'll send it separately.
>
>> Is it worth while to make it inline?
>
> From CodingStyle:
>
> Functions in .c files should not normally be marked "inline", because
> it does not usually help code generation and it does suppress
> compilers warnings about unused functions. (Functions defined in .h
> usually should be marked inline.)
If inline make sense, it should be in a header file. This is not a big deal.
>
>> Acked-by: Andy Zhou <az...@nicira.com>
>
> Thanks, I'll push the series to master soon.
>
>> On Thu, Dec 18, 2014 at 5:11 PM, Joe Stringer <joestrin...@nicira.com> wrote:
>>> Thinking about this a little more, I could make the debug interface
>>> set a single static 'enable_ufid' variable, then have a helper
>>> function like so:
>>>
>>> +static bool
>>> +udpif_use_ufid(struct udpif *udpif)
>>> +{
>>> + return enable_ufid && ofproto_dpif_get_enable_ufid(udpif->backer);
>>> +}
>>>
>>> I don't feel strongly about it, though this is perhaps a little more
>>> intuitive.
>>>
>>> On 18 December 2014 at 16:42, Joe Stringer <joestrin...@nicira.com> wrote:
>>>> Almost; theoretically not all udpifs will have the same backer, each
>>>> of which may or may not support UFID. So, it's still useful to track
>>>> this per-udpif. (One example is right now if you add a userspace
>>>> datapath and a kernel datapath, the userspace will support UFID but
>>>> the kernel will not.)
>>>>
>>>> The rest I think can be implemented by just performing the check
>>>> before setting udpif->enable_ufid to true in the debug interface,
>>>> incremental below:
>>>>
>>>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>>>> index f0cd4cc..46e6ce0 100644
>>>> --- a/ofproto/ofproto-dpif-upcall.c
>>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>>> @@ -2087,11 +2087,20 @@ upcall_unixctl_enable_ufid(struct unixctl_conn
>>>> *conn, int argc OVS_UNUSED,
>>>> const char *argv[] OVS_UNUSED, void *aux
>>>> OVS_UNUSED)
>>>> {
>>>> struct udpif *udpif;
>>>> + bool error = false;
>>>>
>>>> LIST_FOR_EACH (udpif, list_node, &all_udpifs) {
>>>> - atomic_store(&udpif->enable_ufid, true);
>>>> + if (ofproto_dpif_get_enable_ufid(udpif->backer)) {
>>>> + atomic_store(&udpif->enable_ufid, true);
>>>> + } else {
>>>> + error = true;
>>>> + }
>>>> + }
>>>> + if (error) {
>>>> + unixctl_command_reply_error(conn, "One or more datapaths
>>>> don't support UFID");
>>>> + } else {
>>>> + unixctl_command_reply(conn, "Datapath dumping tersely using
>>>> UFID enabled");
>>>> }
>>>> - unixctl_command_reply(conn, "Datapath dumping tersely using UFID
>>>> enabled");
>>>> }
>>>>
>>>> /* Set the flow limit.
>>>>
>>>> On 18 December 2014 at 16:15, Andy Zhou <az...@nicira.com> wrote:
>>>>> It seems we can do away with udpif->enable_ufid field. udpif has
>>>>> access to the backer, so it can know
>>>>> about the datapath capability.
>>>>>
>>>>> The debug interface just need to control another global 'enable'
>>>>> variable. ufid can only be use if both conditions
>>>>> are true. This will also remove the possibility of turning on udpif
>>>>> when the datapath does not support it.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
