Re: [PATCH] tcp: Export to userspace the TCP state names for the trace events

[Yafang Shao]

2017-11-11 3:32 GMT+00:00 Steven Rostedt <rostedt@xxxxxxxxxxx>:
> On Sat, 11 Nov 2017 02:06:00 +0000
> Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
>> 2017-11-10 15:07 GMT+00:00 Steven Rostedt <rostedt@xxxxxxxxxxx>:
>> > On Fri, 10 Nov 2017 12:56:06 +0800
>> > Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>> >
>> >> Could the macro tcp_state_name() be renamed ï
>> >> If <trace/events/tcp.h> is included in include/net/tcp.h, it will
>> >
>> > Ideally, you don't want to include trace/events/*.h headers in other
>> > headers, as they can have side effects if those headers are included in
>> > other trace/events/*.h headers.
>> >
>>
>> Actually I find trace/events/*.h is included in lots of other headers,
>> for example,
>>
>> net/rxrpc/ar-internal.h
>
> This is an internal header, so it's not that likely to be used where it
> shouldn't be.
>
>> include/linux/bpf_trace.h
>> fs/f2fs/trace.h
>
> The above two are actually headers specifically used to pull in the
> trace/events/*.h headers.
>
>> fs/afs/internal.h
>
> another internal header. Unlikely to be misused.
>
>> arch/x86/include/asm/mmu_context.h
>
> This one, hmm, probably should be fixed.
>
>> ...
>>
>> Are these files doing properly ?
>
> Most yes, some probably not.
>
>> Should we fix them ?
>
> Probably, but if they are used incorrectly, it would usually fail on
> build (The same global functions and variables would be defined).
>
>>
>> But per my understanding, it is ok to include  trace/events/*.h in
>> other headers because we defined TRACE_SYSTEM as well, as a
>> consequence those headers should not included in trace/events/*.h. If
>> that happens, it may means that one of the these two TRACE_SYSTEM is
>> not defined properly. Maybe these two TRACE_SYSTEM should be merged to
>> one TRACE_SYSTEM.
>
> Two different files may have the same TRACE_SYSTEM defined. That's not
> an issue.
>
> The issue is, if you have a trace/events/*.h header in a popular file
> (like it use to be in include/linux/slab.h), then it can cause issues
> if another trace/events/*.h header includes it. That's because each
> trace/events/*.h header must be included with CREATE_TRACE_POINTS only
> once.
>

Understood.
Thanks for explanation.

>>
>>
>> >> cause compile error, because there's another function tcp_state_name()
>> >> defined in net/netfilter/ipvs/ip_vs_proto_tcp.c.
>> >>     static const char * tcp_state_name(int state)
>> >>     {
>> >>
>> >>         if (state >= IP_VS_TCP_S_LAST)
>> >>
>> >>             return "ERR!";
>> >>
>> >>         return tcp_state_name_table[state] ? tcp_state_name_table[state] : "?";
>> >>
>> >>     }
>> >
>> > But that said, I didn't make up the trace_state_name(), it was already
>> > there in net-next before this patch.
>> >
>>
>> I know that is not your fault.
>
> :-)
>
>> But as you are modifying this file, it is better to modify it in your
>> patch as well.
>> So we need not submit another new patch to fix it.
>
> I could whip up a patch 2.
>
>>
>> > But yeah, in actuality, I would have just done:
>> >
>> > #define EM(a)   { a, #a },
>> > #define EMe(a)  { a, #a }
>> >
>> > directly. Which we can still do.
>> >
>> > -- Steve
>> >
>>
>> The suggestion from Song is good to fix it.
>
> Song's suggestion seems like it can simple be a patch added on top of
> mine. As it is somewhat agnostic to the fix I'm making. That is, it's a
> different problem, and thus should be a different patch.
>

Got it. These two issues should be fixed in two different patches :-)

Thanks
Yafang