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.)
> 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
