Re: [PATCH v2 0/5] rework access to /proc/net/rpc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2014-12-09 09:42, Timo Teras wrote:
On Tue, 09 Dec 2014 09:16:59 +0100
David Härdeman <david@xxxxxxxxxxx> wrote:

it seems that the "rework access to /proc/net/rpc" patchset removed
dynamic buffers in favour of static, fixed size, buffers. That seems
like a step backwards to me?

Depends a bit on your view. On read() side, readline() like
functionality is removed yes. Though, my understanding is so that this
is not needed with the kernel API. Maybe someone can correct me if I'm
wrong. The removal simplifies memory management, overall code size. As
probably has a positive impact on speed too (probably not too big, but
this communication is used all overall, so it might be useful).

And it makes the buffer size static, introducing an arbitrary limitation (although a rather large one...32KB allocated on the stack IIRC)

On write() side the old code was completely wrong. It did several
assumptions on how FILE buffering works, most of them being incorrect
in general, but also glibc. It only worked because no large messages
have been sent to kernel.

I didn't really check the write() side, it was just the readline() that I was interested in actually...


At least the readline() function could be implemented using
read/write (instead of fread/fwrite) and a dynamic buffer...no?

It's extra complexity. I'd rather not add it unless it's required. My
understanding about the communication mechanism with kernel is that
it's not required. Why have code that would never be used?

I agree that it depends on your view. I tend to be very sceptical of arbitrary limitations unless they have a very good reason (like measurable and relevant performance impact), I doubt that's the case here.

It's up to the maintainer though, I just wanted to point it out :)

Regards,
David

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux