[U-Boot] [PATCH] net: Improve the speed of netconsole

[Joe Hershberger]

Hi Mike,

On Wed, Jul 25, 2012 at 1:49 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Tuesday 24 July 2012 16:11:15 Joe Hershberger wrote:
>> --- a/drivers/net/netconsole.c
>> +++ b/drivers/net/netconsole.c
>> @@ -131,8 +131,17 @@ static void nc_send_packet(const char *buf, int len)
>>       }
>>
>>       if (eth->state != ETH_STATE_ACTIVE) {
>> -             if (eth_init(gd->bd) < 0)
>> -                     return;
>> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>> +             if (net_loop_last_protocol != NETCONS) {
>> +#endif
>> +                     if (eth_init(gd->bd) < 0)
>> +                             return;
>> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>> +                     net_loop_last_protocol = NETCONS;
>> +             } else {
>> +                     eth_init_state_only(gd->bd);
>> +             }
>> +#endif
>
> seems to me these pre/post clauses should really get refactored someway.  the
> ifdef noise is significant here.

I agree.

> so in the header, something like:
>
> static inline int eth_reinit_protocol(int protocol)
> {
> #ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>         return protocol != NETCONS;
> #else
>         return 1;
> #endif
> }
> static inline void eth_set_last_protocol(int protocol)
> {
> #ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>         net_loop_last_protocol = protocol;
> #endif
> }
> #ifdef CONFIG_NETCONSOLE_PERSIST_ETH
> extern enum proto_t net_loop_last_protocol;
> #else
> # define net_loop_last_protocol -1
> #endif
>
> then the source becomes the more manageable:
>
> if (eth_reinit_protocol(net_loop_last_protocol)) {
>         if (eth_init(gd->bd) < 0)
>                 return;
>         eth_set_last_protocol(NETCONS);
> } else
>         eth_init_state_only(gd->bd);

Much nicer.  With static inline it should compile to the same thing.

>> --- a/net/eth.c
>> +++ b/net/eth.c
>>
>> +#ifdef CONFIG_NETCONSOLE_PERSIST_ETH
>> +int eth_init_state_only(bd_t *bis)
>> +{
>> +     eth_current->state = ETH_STATE_ACTIVE;
>> +
>> +     return 0;
>> +}
>> +
>> +void eth_halt_state_only(void)
>> +{
>> +     eth_current->state = ETH_STATE_PASSIVE;
>> +}
>> +#endif
>
> these *really* should be static inlines in the global header.  they're so dirt
> simple, the overhead of the function call is probably much higher than the
> single memory store.

I can do that, but I don't think it will save anything.  Since
eth_current is static, I would have to change it to eth_get_dev(), and
we're back to a function call.  Thoughts?

Thanks,
-Joe