Re: [V9fs-developer] :[RFC] [PATCH 6/7] [net/9p] Read and Write side zerocopy changes for 9P2000.L protocol.

["Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com>]

On 2/8/2011 1:09 PM, Eric Van Hensbergen wrote:
> One thing I wonder is if we always want to zero copy for payload.  In
> the extreme, do we want to take the overhead of pinning an extra page
> if we are only reading/writing a byte?  memcpy is expensive for large
> packets, but may actually be more efficient for small packets.
I am not a memory expert, but I would assume memcpy also need to do
same thing similar to get_user_pages() short of pinning pages. But I see the point.

> 
> Have we done any performance measurements of this code with various
> payload sizes versus non-zero-copy?  Of course that may not really
> show the impact of pinning the extra pages....

All our testing is with large buffers. Did not test with small buffers.

> 
> In any case, if such a tradeoff did exist, we might choose to not do
> zero copy for requests smaller than some size -- and that might
> alleviate some of the problems with the legacy protocols (such as the
> Rerror issue) -- killing two birds with one stone.  In any case, given

I think it is a wise decision to avoid zero copy if iosize+hdr_size <= pagesize.
But It doesn't change any of today's complexity. Except may be saving an if
condition.

> the implementations it should be really easy to shut me up with
> comparison data of zc and non-zc for 1, 64, 128, 256, 512, 1024, 2048,
> 4192 byte payloads (without caches enabled of course).

I think this is good experiment will publish data.

> 
>        -eric
> 
> 
> On Mon, Feb 7, 2011 at 12:57 AM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com> wrote:
>> On 2/6/2011 11:21 PM, Venkateswararao Jujjuri (JV) wrote:
>>> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
>>> ---
>>>  net/9p/client.c   |   45 +++++++++++++++++++++++++++++++--------------
>>>  net/9p/protocol.c |   38 ++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 69 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/net/9p/client.c b/net/9p/client.c
>>> index a848bca..f939edf 100644
>>> --- a/net/9p/client.c
>>> +++ b/net/9p/client.c
>>> @@ -1270,7 +1270,14 @@ p9_client_read(struct p9_fid *fid, char *data, char __user *udata, u64 offset,
>>>       if (count < rsize)
>>>               rsize = count;
>>>
>>> -     req = p9_client_rpc(clnt, P9_TREAD, "dqd", fid->fid, offset, rsize);
>>> +     if ((clnt->trans_mod->pref & P9_TRANS_PREF_PAYLOAD_MASK) ==
>>> +                     P9_TRANS_PREF_PAYLOAD_SEP) {
>>> +             req = p9_client_rpc(clnt, P9_TREAD, "dqE", fid->fid, offset,
>>> +                             rsize, data ? data : udata);
>>> +     } else {
>>> +             req = p9_client_rpc(clnt, P9_TREAD, "dqd", fid->fid, offset,
>>> +                             rsize);
>>> +     }
>>>       if (IS_ERR(req)) {
>>>               err = PTR_ERR(req);
>>>               goto error;
>>> @@ -1284,13 +1291,15 @@ p9_client_read(struct p9_fid *fid, char *data, char __user *udata, u64 offset,
>>>
>>>       P9_DPRINTK(P9_DEBUG_9P, "<<< RREAD count %d\n", count);
>>>
>>> -     if (data) {
>>> -             memmove(data, dataptr, count);
>>> -     } else {
>>> -             err = copy_to_user(udata, dataptr, count);
>>> -             if (err) {
>>> -                     err = -EFAULT;
>>> -                     goto free_and_error;
>>> +     if (!req->tc->pbuf_size) {
>>> +             if (data) {
>>> +                     memmove(data, dataptr, count);
>>> +             } else {
>>> +                     err = copy_to_user(udata, dataptr, count);
>>> +                     if (err) {
>>> +                             err = -EFAULT;
>>> +                             goto free_and_error;
>>> +                     }
>>>               }
>>>       }
>>>       p9_free_req(clnt, req);
>>> @@ -1323,12 +1332,20 @@ p9_client_write(struct p9_fid *fid, char *data, const char __user *udata,
>>>
>>>       if (count < rsize)
>>>               rsize = count;
>>> -     if (data)
>>> -             req = p9_client_rpc(clnt, P9_TWRITE, "dqD", fid->fid, offset,
>>> -                                                             rsize, data);
>>> -     else
>>> -             req = p9_client_rpc(clnt, P9_TWRITE, "dqU", fid->fid, offset,
>>> -                                                             rsize, udata);
>>> +
>>> +     if ((clnt->trans_mod->pref & P9_TRANS_PREF_PAYLOAD_MASK) ==
>>> +                     P9_TRANS_PREF_PAYLOAD_SEP) {
>>> +             req = p9_client_rpc(clnt, P9_TWRITE, "dqF", fid->fid, offset,
>>> +                             rsize, data ? data : udata);
>>> +     } else {
>>> +             if (data)
>>> +                     req = p9_client_rpc(clnt, P9_TWRITE, "dqD", fid->fid,
>>> +                                     offset, rsize, data);
>>> +             else
>>> +                     req = p9_client_rpc(clnt, P9_TWRITE, "dqU", fid->fid,
>>> +                                     offset, rsize, udata);
>>> +     }
>>> +
>>>       if (IS_ERR(req)) {
>>>               err = PTR_ERR(req);
>>>               goto error;
>>> diff --git a/net/9p/protocol.c b/net/9p/protocol.c
>>> index dfc358f..ea778dd 100644
>>> --- a/net/9p/protocol.c
>>> +++ b/net/9p/protocol.c
>>> @@ -114,6 +114,24 @@ pdu_write_u(struct p9_fcall *pdu, const char __user *udata, size_t size)
>>>       return size - len;
>>>  }
>>>
>>> +static size_t
>>> +pdu_write_uw(struct p9_fcall *pdu, const char *udata, size_t size)
>>> +{
>>> +     size_t len = min(pdu->capacity - pdu->size, size);
>>> +     pdu->pbuf = udata;
>>> +     pdu->pbuf_size = len;
>>> +     return size - len;
>>> +}
>>> +
>>> +static size_t
>>> +pdu_write_ur(struct p9_fcall *pdu, const char *udata, size_t size)
>>> +{
>>> +     size_t len = min(pdu->capacity - pdu->size, size);
>>> +     pdu->pbuf = udata;
>>> +     pdu->pbuf_size = len;
>>> +     return size - len;
>>> +}
>>> +
>>>  /*
>>>       b - int8_t
>>>       w - int16_t
>>> @@ -445,6 +463,26 @@ p9pdu_vwritef(struct p9_fcall *pdu, int proto_version, const char *fmt,
>>>                                       errcode = -EFAULT;
>>>                       }
>>>                       break;
>>> +             case 'E':{
>>> +                              int32_t count = va_arg(ap, int32_t);
>>> +                              const char *udata = va_arg(ap, const void *);
>>> +                              errcode = p9pdu_writef(pdu, proto_version, "d",
>>> +                                              count);
>>> +                              if (!errcode && pdu_write_ur(pdu, udata,
>>> +                                                      count))
>>> +                                      errcode = -EFAULT;
>>> +                      }
>>> +                      break;
>>> +             case 'F':{
>>> +                              int32_t count = va_arg(ap, int32_t);
>>> +                              const char *udata = va_arg(ap, const void *);
>>> +                              errcode = p9pdu_writef(pdu, proto_version, "d",
>>> +                                              count);
>>> +                              if (!errcode && pdu_write_uw(pdu, udata,
>>> +                                                      count))
>>> +                                      errcode = -EFAULT;
>>> +                      }
>>> +                      break;
>>>               case 'U':{
>>>                               int32_t count = va_arg(ap, int32_t);
>>>                               const char __user *udata =
>>
>>
>>
>> ------------------------------------------------------------------------------
>> The modern datacenter depends on network connectivity to access resources
>> and provide services. The best practices for maximizing a physical server's
>> connectivity to a physical network are well understood - see how these
>> rules translate into the virtual world?
>> http://p.sf.net/sfu/oracle-sfdevnlfb
>> _______________________________________________
>> V9fs-developer mailing list
>> V9fs-developer@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/v9fs-developer
>>