netdev - Re: [PATCH net-next v4 05/10] tsnep: Add XDP TX support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Jan 2023 19:51:26 +0100
From:   Gerhard Engleder <gerhard@...leder-embedded.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        edumazet@...gle.com, pabeni@...hat.com
Subject: Re: [PATCH net-next v4 05/10] tsnep: Add XDP TX support

On 10.01.23 23:38, Alexander Duyck wrote:
> On Tue, Jan 10, 2023 at 1:07 PM Gerhard Engleder
> <gerhard@...leder-embedded.com> wrote:
>>
>> On 10.01.23 17:56, Alexander H Duyck wrote:
>>> nOn Mon, 2023-01-09 at 20:15 +0100, Gerhard Engleder wrote:
>>>> Implement ndo_xdp_xmit() for XDP TX support. Support for fragmented XDP
>>>> frames is included.
>>>>
>>>> Also some const, braces and logic clean ups are done in normal TX path
>>>> to keep both TX paths in sync.
>>>>
>>>> Signed-off-by: Gerhard Engleder <gerhard@...leder-embedded.com>

<...>

>>>> -static int tsnep_tx_map(struct sk_buff *skb, struct tsnep_tx *tx, int count)
>>>> +static int tsnep_tx_map(const struct sk_buff *skb, struct tsnep_tx *tx,
>>>> +                    int count)
>>>>    {
>>>
>>> This change to const doesn't add anything since this is a static
>>> function. You could probably just skip making this change since the
>>> function will likely be inlined anyway.
>>
>> const was requested for tsnep_xdp_tx_map() during last review round so I
>> added it also here to keep both function similar.
> 
> As a general rule it doesn't add anything to make an argument to a
> static function const unless the callers are also making it a const.
> Otherwise what you end up doing is just adding useless modifiers that
> will be thrown away to the code as the compiler can already take care
> of whatever optimizations it can get out of it.

For me removing const is totally ok. I will remove it as I see no value
for static functions too. Let's see what the next reviewer says ;-)

<...>

>>>> +static int tsnep_xdp_tx_map(const struct xdp_frame *xdpf, struct tsnep_tx *tx,
>>>> +                        const struct skb_shared_info *shinfo, int count,
>>>> +                        enum tsnep_tx_type type)
>>>
>>> Again the const here isn't adding any value since this is a static
>>> function and will likely be inlined into the function below which calls
>>> it.
>>
>> const was requested here during last review round so I added it. It may
>> add some value by detecting some problems at compile time.
> 
> I suppose, but really adding a const attribute here doesn't add much
> here unless you are also going to enforce it at higher levels such as
> the xmit_frame_ring function itself. Also keep in mind that all the
> const would protect is the xdp frame structure itself. It does nothing
> to keep us from modifying the data in the pages and such.

Of course.

Gerhard