Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20171110223206.5b5f79d8@gandalf.local.home>
References: <1510207298-14828-1-git-send-email-laoar.shao@gmail.com>
 <20171109064326.skfvvohg7qhgl3lp@ast-mbp> <20171109131808.373c296f@gandalf.local.home>
 <30A95900-7378-439F-954E-BD5487012435@fb.com> <CFF85160-E239-4E15-8438-CB8C2795D4FD@fb.com>
 <20171109195712.47fe23b6@gandalf.local.home> <CALOAHbCM1d8rs1_KMo0MMCwgTG18TeiH_ZQJ2=sgPst2PoU9Xg@mail.gmail.com>
 <20171110100700.5870c21f@gandalf.local.home> <CALOAHbD16KNZ4JWTLudQLDgB72Ke7Twdb-bLYznH7voAUQvQog@mail.gmail.com>
 <20171110223206.5b5f79d8@gandalf.local.home>
From:   Yafang Shao <laoar.shao@gmail.com>
Date:   Sat, 11 Nov 2017 13:26:38 +0000
Message-ID: <CALOAHbC4=qVwMza76qvZTj+nfPO5WhbU0dJ9ixTL-xjP5edFbA@mail.gmail.com>
Subject: Re: [PATCH] tcp: Export to userspace the TCP state names for the
 trace events
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     David Miller <davem@davemloft.net>,
        Song Liu <songliubraving@fb.com>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

2017-11-11 3:32 GMT+00:00 Steven Rostedt <rostedt@goodmis.org>:
> On Sat, 11 Nov 2017 02:06:00 +0000
> Yafang Shao <laoar.shao@gmail.com> wrote:
>
>> 2017-11-10 15:07 GMT+00:00 Steven Rostedt <rostedt@goodmis.org>:
>> > On Fri, 10 Nov 2017 12:56:06 +0800
>> > Yafang Shao <laoar.shao@gmail.com> wrote:
>> >
>> >> Could the macro tcp_state_name() be renamed =EF=BC=9F
>> >> 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 i=
n
>> > 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 >=3D IP_VS_TCP_S_LAST)
>> >>
>> >>             return "ERR!";
>> >>
>> >>         return tcp_state_name_table[state] ? tcp_state_name_table[sta=
te] : "?";
>> >>
>> >>     }
>> >
>> > 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

From 1583738972392359262@xxx Sat Nov 11 03:33:01 +0000 2017
X-GM-THRID: 1583638629223849157
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread