On Sun, Jul 26, 2020 at 9:55 AM Toshiaki Makita
<toshiaki.maki...@gmail.com> wrote:
>
snip
> >> How about doing s.t like:
> >> --enable-afxdp: the current one on master without the xdp offload program
> >> --enable-afxdp-with-bpf: the afxdp one plus your xdp offload program
> >>
> >> So that when users only --enable-afxdp, it doesn't need to compile in the
> >> lib/netdev-offload-xdp.* and bpf/*
> >
> > Let me clarify it.
> >
> > Currently,
> >
> > --enable-afxdp: build lib/netdev-afxdp* and lib/netdev-offload-xdp*
> > --enable-bpf: build bpf/*
> >
> > Do you sugguest this?
> >
> > --enable-afxdp: build lib/netdev-afxdp*
> > --enable-afxdp-with-bpf: build lib/netdev-afxdp*, lib/netdev-offload-xdp*,
> > and bpf/*
>
> Maybe we should not introduce this kind of overlapping?
> (enable-afxdp-with-bpf should not include enable-afxdp)
>
> How about this?
>
> --enable-afxdp: build lib/netdev-afxdp*
> --enable-xdp-offload: build lib/netdev-offload-xdp*
> --enable-bpf: build bpf/*
>
> I thought the following is also possible:
>
> --enable-afxdp: build lib/netdev-afxdp*
> --enable-afxdp=offload: build lib/netdev-afxdp* and lib/netdev-offload-xdp*
> --enable-bpf: build bpf/*
But isn't --enable-afxdp-offload has dependency on --enable-bpf?
Otherwise, if only doing "--enable-afxdp-offload", there is no BPF program
compiled and nothing is loaded.
>
> But theoretically xdp offload can be enabled without afxdp in the future,
> so I guess a different build switch, enable-xdp-offload, is better.
>
> >>> +static int
> >>> +xdp_preload(struct netdev *netdev, struct bpf_object *obj)
> ...
> >>> + if (ovsthread_once_start(&output_map_once)) {
> >>> + /* XXX: close output_map_fd somewhere? */
> >> maybe at uninit_flow_api, we have to check whether there is
> >> still a netdev using linux_xdp offload. And if not, close this map?
> >
> > Then we need to cancel the ovsthread_once at that time?
> > I could not find such an api.
> >
> > I feel like we should close the map when shutting down the ovs-vswitchd,
> > but not so
> > important
> > as the process exits anyway.
>
> I'll just drop this comment. This should not be necessary.
>
> >>> +static int
> >>> +netdev_xdp_flow_put(struct netdev *netdev, struct match *match_,
> ...
> >>> + memcpy(pentry, entry, netdev_info->subtable_mask_size);
> >>> + pidx = idx;
> >>> + idx = entry->next;
> >> question about this for loop:
> >> why do we need to maintain ->next for the struct
> >> xdp_subtable_mask_header *entry?
> >> I thought we have a fixed-sized array of subtable.
> >> And all we need to do is go over all the valid index of the array,
> >> until finding a match.
> >
> > I think you are right.
> > This code is ported from xdp_flow, where order is necessary.
> > I guess ovs-vswitchd does not care the order of each masks.
> > Will double check it.
>
> After some more consideration, I'm thinking using ->next to maintain a list
> is better.
>
> As you say, indeed we can maintain an array to hold masks list.
> But in that case we need to have a feature to invalidate an entry if we don't
> replace the entire array on deletion of an entry.
> Also to avoid too sparse array, we probably should implement deletion by
> swapping
> the entry to be deleted and the last entry. Then, we need to invalidate an
> entry
> temporarily, since we don't want to access the entry while it is being
> updated.
>
> Think about something like the following entry to do invalidation:
>
> struct table_entry {
> struct entry_value value;
> uint8_t valid;
> }
>
> We should update ->valid field and ->value field separately in order not to
> let CPU
> reorder memory read (bpf does not provide memory barriers).
yes, that's a big limitation.
>
> in vswitchd:
>
> entry->valid = false;
> bpf_map_update_elem(map, idx, entry);
> entry->value.x = x;
> entry->value.y = y;
> ...
> bpf_map_update_elem(map, idx, entry);
> entry->valid = true;
> bpf_map_update_elem(map, idx, entry);
I see, thanks for your explanation.
William
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
