Re: [PATCH RFC 0/3] tun zerocopy stats

[Jason Wang]

On 2017年10月12日 05:44, Willem de Bruijn wrote:

On Tue, Oct 10, 2017 at 11:15 PM, Jason Wang  wrote:


On 2017年10月11日 03:11, Willem de Bruijn wrote:

On Tue, Oct 10, 2017 at 1:39 PM, David Miller  wrote:

From: Willem de Bruijn 
Date: Tue, 10 Oct 2017 11:29:33 -0400


If there is a way to expose these stats through vhost_net directly,
instead of through tun, that may be better. But I did not see a
suitable interface. Perhaps debugfs.

Please don't use debugfs, thank you :-)

Okay. I'll take a look at tracing for on-demand measurement.


This reminds me a past series that adding tracepoints to vhost/net[1]. It
can count zero/datacopy independently and even contains a sample program to
show the stats.

Interesting, thanks!

For occasional evaluation, we can also use a bpf kprobe for the time being:

   bpf_program = """
   #include 
   #include 

   BPF_ARRAY(count, u64, 2);

   void inc_counter(struct pt_regs *ctx) {
   bool success;
   int key;
   u64 *val;

   success = PT_REGS_PARM2(ctx);
   key = success ? 0 : 1;
   val = count.lookup();
   if (val)
   lock_xadd(val, 1);
   }
   """

   b = bcc.BPF(text=bpf_program)
   b.attach_kprobe(event="vhost_zerocopy_callback", fn_name="inc_counter")

   time.sleep(5)

   print("vhost_zerocopy_callback: Y:%d N:%d" %
 (b["count"][ctypes.c_int(0)].value,
  b["count"][ctypes.c_int(1)].value))


Thanks for the tips, looks flexible.

Re: [PATCH RFC 0/3] tun zerocopy stats

[Willem de Bruijn]

On Tue, Oct 10, 2017 at 11:15 PM, Jason Wang  wrote:
>
>
> On 2017年10月11日 03:11, Willem de Bruijn wrote:
>>
>> On Tue, Oct 10, 2017 at 1:39 PM, David Miller  wrote:
>>>
>>> From: Willem de Bruijn 
>>> Date: Tue, 10 Oct 2017 11:29:33 -0400
>>>
 If there is a way to expose these stats through vhost_net directly,
 instead of through tun, that may be better. But I did not see a
 suitable interface. Perhaps debugfs.
>>>
>>> Please don't use debugfs, thank you :-)
>>
>> Okay. I'll take a look at tracing for on-demand measurement.
>
>
> This reminds me a past series that adding tracepoints to vhost/net[1]. It
> can count zero/datacopy independently and even contains a sample program to
> show the stats.

Interesting, thanks!

For occasional evaluation, we can also use a bpf kprobe for the time being:

  bpf_program = """
  #include 
  #include 

  BPF_ARRAY(count, u64, 2);

  void inc_counter(struct pt_regs *ctx) {
  bool success;
  int key;
  u64 *val;

  success = PT_REGS_PARM2(ctx);
  key = success ? 0 : 1;
  val = count.lookup();
  if (val)
  lock_xadd(val, 1);
  }
  """

  b = bcc.BPF(text=bpf_program)
  b.attach_kprobe(event="vhost_zerocopy_callback", fn_name="inc_counter")

  time.sleep(5)

  print("vhost_zerocopy_callback: Y:%d N:%d" %
(b["count"][ctypes.c_int(0)].value,
 b["count"][ctypes.c_int(1)].value))

Re: [PATCH RFC 0/3] tun zerocopy stats

[Jason Wang]

On 2017年10月11日 03:11, Willem de Bruijn wrote:

On Tue, Oct 10, 2017 at 1:39 PM, David Miller  wrote:

From: Willem de Bruijn 
Date: Tue, 10 Oct 2017 11:29:33 -0400


If there is a way to expose these stats through vhost_net directly,
instead of through tun, that may be better. But I did not see a
suitable interface. Perhaps debugfs.

Please don't use debugfs, thank you :-)

Okay. I'll take a look at tracing for on-demand measurement.


This reminds me a past series that adding tracepoints to vhost/net[1]. 
It can count zero/datacopy independently and even contains a sample 
program to show the stats.


Thanks

[1] https://lists.oasis-open.org/archives/virtio-dev/201403/msg00025.html

Re: [PATCH RFC 0/3] tun zerocopy stats

[Willem de Bruijn]

On Tue, Oct 10, 2017 at 1:39 PM, David Miller  wrote:
> From: Willem de Bruijn 
> Date: Tue, 10 Oct 2017 11:29:33 -0400
>
>> If there is a way to expose these stats through vhost_net directly,
>> instead of through tun, that may be better. But I did not see a
>> suitable interface. Perhaps debugfs.
>
> Please don't use debugfs, thank you :-)

Okay. I'll take a look at tracing for on-demand measurement.

Re: [PATCH RFC 0/3] tun zerocopy stats

[David Miller]

From: Willem de Bruijn 
Date: Tue, 10 Oct 2017 11:29:33 -0400

> If there is a way to expose these stats through vhost_net directly,
> instead of through tun, that may be better. But I did not see a
> suitable interface. Perhaps debugfs.

Please don't use debugfs, thank you :-)

Re: [PATCH RFC 0/3] tun zerocopy stats

[Stephen Hemminger]

On Tue, 10 Oct 2017 11:29:33 -0400
Willem de Bruijn  wrote:

> On Mon, Oct 9, 2017 at 11:52 PM, David Miller  wrote:
> > From: Willem de Bruijn 
> > Date: Fri,  6 Oct 2017 18:25:13 -0400
> >  
> >> From: Willem de Bruijn 
> >>
> >> Add zerocopy transfer statistics to the vhost_net/tun zerocopy path.
> >>
> >> I've been using this to verify recent changes to zerocopy tuning [1].
> >> Sharing more widely, as it may be useful in similar future work.
> >>
> >> Use ethtool stats as interface, as these are defined per device
> >> driver and can easily be extended.
> >>
> >> Make the zerocopy release callback take an extra hop through the tun
> >> driver to allow the driver to increment its counters.
> >>
> >> Care must be taken to avoid adding an alloc/free to this hot path.
> >> Since the caller already must allocate a ubuf_info, make it allocate
> >> two at a time and grant one to the tun device.
> >>
> >>  1/3: introduce ethtool stats (`ethtool -S $DEV`) for tun devices
> >>  2/3: add zerocopy tx and tx_err counters
> >>  3/3: convert vhost_net to pass a pair of ubuf_info to tun
> >>
> >> [1] http://patchwork.ozlabs.org/patch/822613/  
> >
> > This looks mostly fine to me, but I don't know enough about how vhost
> > and tap interact to tell whether this makes sense to upstream.  
> 
> Thanks for taking a look. The need for monitoring these stats has
> come up in a couple of patch evaluation discussions, so I wanted
> to share at least one implementation to get the data.
> 
> Because the choice to use zerocopy is based on heuristics and
> there is a cost if it mispredicts, I think we even want to being able
> to continuously monitor this in production.
> 
> The implementation is probably not ready for that as is.

Another alternative is to use tracepoints for this.
If you need statistics in production then per-cpu (or per-queue) stats
would have less impact. Tracepoints have no visible impact unless used.

Re: [PATCH RFC 0/3] tun zerocopy stats

[Willem de Bruijn]

On Mon, Oct 9, 2017 at 11:52 PM, David Miller  wrote:
> From: Willem de Bruijn 
> Date: Fri,  6 Oct 2017 18:25:13 -0400
>
>> From: Willem de Bruijn 
>>
>> Add zerocopy transfer statistics to the vhost_net/tun zerocopy path.
>>
>> I've been using this to verify recent changes to zerocopy tuning [1].
>> Sharing more widely, as it may be useful in similar future work.
>>
>> Use ethtool stats as interface, as these are defined per device
>> driver and can easily be extended.
>>
>> Make the zerocopy release callback take an extra hop through the tun
>> driver to allow the driver to increment its counters.
>>
>> Care must be taken to avoid adding an alloc/free to this hot path.
>> Since the caller already must allocate a ubuf_info, make it allocate
>> two at a time and grant one to the tun device.
>>
>>  1/3: introduce ethtool stats (`ethtool -S $DEV`) for tun devices
>>  2/3: add zerocopy tx and tx_err counters
>>  3/3: convert vhost_net to pass a pair of ubuf_info to tun
>>
>> [1] http://patchwork.ozlabs.org/patch/822613/
>
> This looks mostly fine to me, but I don't know enough about how vhost
> and tap interact to tell whether this makes sense to upstream.

Thanks for taking a look. The need for monitoring these stats has
come up in a couple of patch evaluation discussions, so I wanted
to share at least one implementation to get the data.

Because the choice to use zerocopy is based on heuristics and
there is a cost if it mispredicts, I think we even want to being able
to continuously monitor this in production.

The implementation is probably not ready for that as is.

> What are the runtime costs for these new statistics?

It currently doubles the size of the ubuf_info memory pool. That can be
fixed, as the current size is UIO_MAXIOV (1024), but the number of
zerocopy packets in flight is bound by VHOST_MAX_PEND (128).

It also adds an indirect function call to call to each zerocopy skb free
path, though.

If there is a way to expose these stats through vhost_net directly,
instead of through tun, that may be better. But I did not see a
suitable interface. Perhaps debugfs.

Re: [PATCH RFC 0/3] tun zerocopy stats

[David Miller]

From: Willem de Bruijn 
Date: Fri,  6 Oct 2017 18:25:13 -0400

> From: Willem de Bruijn 
> 
> Add zerocopy transfer statistics to the vhost_net/tun zerocopy path.
> 
> I've been using this to verify recent changes to zerocopy tuning [1].
> Sharing more widely, as it may be useful in similar future work.
> 
> Use ethtool stats as interface, as these are defined per device
> driver and can easily be extended.
> 
> Make the zerocopy release callback take an extra hop through the tun
> driver to allow the driver to increment its counters.
> 
> Care must be taken to avoid adding an alloc/free to this hot path.
> Since the caller already must allocate a ubuf_info, make it allocate
> two at a time and grant one to the tun device.
> 
>  1/3: introduce ethtool stats (`ethtool -S $DEV`) for tun devices
>  2/3: add zerocopy tx and tx_err counters
>  3/3: convert vhost_net to pass a pair of ubuf_info to tun
> 
> [1] http://patchwork.ozlabs.org/patch/822613/

This looks mostly fine to me, but I don't know enough about how vhost
and tap interact to tell whether this makes sense to upstream.

What are the runtime costs for these new statistics?

[PATCH RFC 0/3] tun zerocopy stats

[Willem de Bruijn]

From: Willem de Bruijn 

Add zerocopy transfer statistics to the vhost_net/tun zerocopy path.

I've been using this to verify recent changes to zerocopy tuning [1].
Sharing more widely, as it may be useful in similar future work.

Use ethtool stats as interface, as these are defined per device
driver and can easily be extended.

Make the zerocopy release callback take an extra hop through the tun
driver to allow the driver to increment its counters.

Care must be taken to avoid adding an alloc/free to this hot path.
Since the caller already must allocate a ubuf_info, make it allocate
two at a time and grant one to the tun device.

 1/3: introduce ethtool stats (`ethtool -S $DEV`) for tun devices
 2/3: add zerocopy tx and tx_err counters
 3/3: convert vhost_net to pass a pair of ubuf_info to tun

[1] http://patchwork.ozlabs.org/patch/822613/

Willem de Bruijn (3):
  tun: ethtool stats
  tun: expand ethtool stats with zerocopy
  vhost_net: support tun zerocopy stats

 drivers/net/tun.c   | 111 
 drivers/vhost/net.c |   7 ++--
 2 files changed, 108 insertions(+), 10 deletions(-)

-- 
2.14.2.920.gcf0c67979c-goog