lkml.org 
[lkml]   [2022]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] [openvswitch v4] openvswitch: Add support to count upcall packets
Date
From: wangchuanlei <wangchuanlei@inspur.com>
Date: Wed, 23 Nov 2022 21:24:16 -0500

> Hi,
> Thank you for review! I will give a new verson of patch based on your comments,
> and i give a explanation on every comments from you, please see below!

Oh, just noticed, the subject prefix [openvswitch] is not correct,
please use [PATCH net-next v5] next time.

>
> Best reagrds!
> wangchuanlei

[...]

> > + const struct vport *p = OVS_CB(skb)->input_vport;
> > + struct vport_upcall_stats_percpu *vport_stats;
> > +
> > + vport_stats = this_cpu_ptr(p->vport_upcall_stats_percpu);
>
> Why make a separate structure? You can just expand dp_stats_percpu, this function would then be just a couple lines in ovs_dp_upcall().
> -- emm, beacause of this statistics based on vport, so new structure should insert to "struct vport"

Ah, my bad. Didn't notice that :')

>
>
> > + u64_stats_update_begin(&vport_stats->syncp);
> > + if (upcall_success)
> > + u64_stats_inc(&vport_stats->n_upcall_success);
> > + else
> > + u64_stats_inc(&vport_stats->n_upcall_fail);
> > + u64_stats_update_end(&vport_stats->syncp);
> > + }
> > +}
> > +
> > void ovs_dp_detach_port(struct vport *p) {
> > ASSERT_OVSL();
> > @@ -216,6 +235,9 @@ void ovs_dp_detach_port(struct vport *p)
> > /* First drop references to device. */
> > hlist_del_rcu(&p->dp_hash_node);
> >
> > + /* Free percpu memory */
> > + free_percpu(p->vport_upcall_stats_percpu);
> > +
> > /* Then destroy it. */
> > ovs_vport_del(p);
> > }
> > @@ -305,6 +327,8 @@ int ovs_dp_upcall(struct datapath *dp, struct sk_buff *skb,
> > err = queue_userspace_packet(dp, skb, key, upcall_info, cutlen);
> > else
> > err = queue_gso_packets(dp, skb, key, upcall_info, cutlen);
> > +
> > + ovs_vport_upcalls(skb, upcall_info, !err);
> > if (err)
> > goto err;
>
> Also, as you may see, your ::upcall_fail counter will be always exactly the same as stats->n_lost. So there's no point introducing a new one.
> However, you can expand the structure dp_stats_percpu and add a new field there which would store the number of successfull upcalls.
> ...but I don't see a reason for this to be honest. From my PoV, it's better to count the number of successfully processed packets at the end of queue_userspace_packet() right before the 'out:'
> label[0]. But please make sure then you don't duplicate some other counter (I'm not deep into OvS, so can't say for sure if there's anything similar to what you want).
> --in ovs , as stats->n_lost only count the sum of packets of all ports, not on individal port , so expand the structure dp_stats_percpu may be not suitable
> --and count upcall failed packets is useful beacuse no all of upcall packets are successfully sent。

Yes, I see now, thanks for the explanation! I think it's good idea
in general to introduce OvS per-vport stats. There are some, but
they're stored in net_device::dev_stats, which I'm not a fan of :D

>
> >
> > @@ -1825,6 +1849,13 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
> > goto err_destroy_portids;
> > }
> >
> > + vport->vport_upcall_stats_percpu =
>
> This can be at least twice shorter, e.g. 'upcall_stats'. Don't try to describe every detail in symbol names.
> --yes!
> > + netdev_alloc_pcpu_stats(struct vport_upcall_stats_percpu);
> > + if (!vport->vport_upcall_stats_percpu) {
> > + err = -ENOMEM;
> > + goto err_destroy_upcall_stats;
>
> I know you followed the previous label logics, but you actually aren't destroying the stats under this label. Here you should have `goto err_destroy_portids` as that's what you're actually doing on that error path.
> --here is just keep format of code, and has no influence on function

Correct, so you can use the already existing label here.

>
> > + }
> > +
> > err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
> > info->snd_seq, 0, OVS_DP_CMD_NEW);
> > BUG_ON(err < 0);
>
> [...]
>
> > @@ -2278,6 +2321,14 @@ static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
> > goto exit_unlock_free;
> > }
> >
> > + vport->vport_upcall_stats_percpu =
> > + netdev_alloc_pcpu_stats(struct vport_upcall_stats_percpu);
> > +
> > + if (!vport->vport_upcall_stats_percpu) {
> > + err = -ENOMEM;
> > + goto exit_unlock_free;
> > + }
>
> Why do you allocate them twice?
> -- here is in different code segment on in vport_cmd_new , the other is in dp_cmd_new, they are has no collisions

+ (resolved)

>
> > +
> > err = ovs_vport_cmd_fill_info(vport, reply, genl_info_net(info),
> > info->snd_portid, info->snd_seq, 0,
> > OVS_VPORT_CMD_NEW, GFP_KERNEL);

[...]

> > --
> > 2.27.0
>
> [0] https://elixir.bootlin.com/linux/v6.1-rc6/source/net/openvswitch/datapath.c#L557
>
> Thanks,
> Olek

Thanks,
Olek

\
 
 \ /
  Last update: 2022-11-24 18:34    [W:0.238 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site