DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] RFC - adding filter to packet capture API
@ 2019-12-06 22:11 Stephen Hemminger
  2019-12-09 10:24 ` Ray Kinsella
  2019-12-09 13:41 ` Ananyev, Konstantin
  0 siblings, 2 replies; 5+ messages in thread
From: Stephen Hemminger @ 2019-12-06 22:11 UTC (permalink / raw)
  To: Ray Kinsella; +Cc: dev

In the process of updating packet capture to take a filter program, there
is one open question about API/ABI.

The example is:

int
rte_pdump_enable(uint16_t port, uint16_t queue, uint32_t flags,
		struct rte_ring *ring,
		struct rte_mempool *mp,
		void *filter);

For the new version want to add ability to pass a BPF (classic) program
from libcap. This is described in most pcap API's as "struct bpf_program".

The filter parameter was left as a placeholder, but in typical YAGNI
fashion. When we do need to use it, it is not going to work out.

Since the existing filter argument was never used, there are a bunch
of options (in order from best to worse).

1. Introduce new API which takes a filter. 

int
rte_pdump_enable_bpf(uint16_t port, uint16_t queue, uint32_t flags,
		struct rte_ring *ring,
		struct rte_mempool *mp,
		const struct bpf_program *filter);

The old API is just the same as calling new API with NULL as filter.
This solution is safe but adds minor bloat.

2. Use API versioning.  This solves the ABI problem but it is still
   an API breakage since program that was passing junk would still
   not compile.

3. Change the function signature of existing API. This risks breaking
   at compile time some program that was passing some other value.
   Similarly, a program could have passed garbage, we never checked.

4. Keep existing function signature, but be type unsafe.
   This keeps API, but still is ABI breakage for programs that passed
   garbage. Plus C is unsafe enough already.






^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] RFC - adding filter to packet capture API
  2019-12-06 22:11 [dpdk-dev] RFC - adding filter to packet capture API Stephen Hemminger
@ 2019-12-09 10:24 ` Ray Kinsella
  2019-12-09 13:41 ` Ananyev, Konstantin
  1 sibling, 0 replies; 5+ messages in thread
From: Ray Kinsella @ 2019-12-09 10:24 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev



On 06/12/2019 22:11, Stephen Hemminger wrote:
> In the process of updating packet capture to take a filter program, there
> is one open question about API/ABI.
> 
> The example is:
> 
> int
> rte_pdump_enable(uint16_t port, uint16_t queue, uint32_t flags,
> 		struct rte_ring *ring,
> 		struct rte_mempool *mp,
> 		void *filter);
> 
> For the new version want to add ability to pass a BPF (classic) program
> from libcap. This is described in most pcap API's as "struct bpf_program".
> 
> The filter parameter was left as a placeholder, but in typical YAGNI
> fashion. When we do need to use it, it is not going to work out.
> 
> Since the existing filter argument was never used, there are a bunch
> of options (in order from best to worse).
> 
> 1. Introduce new API which takes a filter. 
> 
> int
> rte_pdump_enable_bpf(uint16_t port, uint16_t queue, uint32_t flags,
> 		struct rte_ring *ring,
> 		struct rte_mempool *mp,
> 		const struct bpf_program *filter);
> 
> The old API is just the same as calling new API with NULL as filter.
> This solution is safe but adds minor bloat.
> 
> 2. Use API versioning.  This solves the ABI problem but it is still
>    an API breakage since program that was passing junk would still
>    not compile.

I honestly think this is the best option.
Our commitment with ABI Stability is only not to break applications linked against 19.11.

If someone is rebuilding against v20.02 and is passing something into the void *.
It is probably no-harm the compiler is giving them an FYI that things have changed.

> 
> 3. Change the function signature of existing API. This risks breaking
>    at compile time some program that was passing some other value.
>    Similarly, a program could have passed garbage, we never checked.
> 
> 4. Keep existing function signature, but be type unsafe.
>    This keeps API, but still is ABI breakage for programs that passed
>    garbage. Plus C is unsafe enough already.

True, but implementing ABI versioning is really trivial in this case, and avoids this.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] RFC - adding filter to packet capture API
  2019-12-06 22:11 [dpdk-dev] RFC - adding filter to packet capture API Stephen Hemminger
  2019-12-09 10:24 ` Ray Kinsella
@ 2019-12-09 13:41 ` Ananyev, Konstantin
  2019-12-09 16:49   ` Stephen Hemminger
  2019-12-11 20:13   ` Morten Brørup
  1 sibling, 2 replies; 5+ messages in thread
From: Ananyev, Konstantin @ 2019-12-09 13:41 UTC (permalink / raw)
  To: Stephen Hemminger, Ray Kinsella; +Cc: dev


> In the process of updating packet capture to take a filter program, there
> is one open question about API/ABI.
> 
> The example is:
> 
> int
> rte_pdump_enable(uint16_t port, uint16_t queue, uint32_t flags,
> 		struct rte_ring *ring,
> 		struct rte_mempool *mp,
> 		void *filter);
> 
> For the new version want to add ability to pass a BPF (classic) program
> from libcap. This is described in most pcap API's as "struct bpf_program".
> 
> The filter parameter was left as a placeholder, but in typical YAGNI
> fashion. When we do need to use it, it is not going to work out.
> 
> Since the existing filter argument was never used, there are a bunch
> of options (in order from best to worse).
> 
> 1. Introduce new API which takes a filter.
> 
> int
> rte_pdump_enable_bpf(uint16_t port, uint16_t queue, uint32_t flags,
> 		struct rte_ring *ring,
> 		struct rte_mempool *mp,
> 		const struct bpf_program *filter);
> 
> The old API is just the same as calling new API with NULL as filter.
> This solution is safe but adds minor bloat.
> 
> 2. Use API versioning.  This solves the ABI problem but it is still
>    an API breakage since program that was passing junk would still
>    not compile.
> 
> 3. Change the function signature of existing API. This risks breaking
>    at compile time some program that was passing some other value.
>    Similarly, a program could have passed garbage, we never checked.
> 
> 4. Keep existing function signature, but be type unsafe.
>    This keeps API, but still is ABI breakage for programs that passed
>    garbage. Plus C is unsafe enough already.
> 

My preference is probably #4, with some extra changes:
make actual type for 'filter' be determined by flags,
something like:

enum {
        RTE_PDUMP_FLAG_RX = 1,  /* receive direction */
        RTE_PDUMP_FLAG_TX = 2,  /* transmit direction */
+      RTE_PDUMP_FLAG_CBPF = 4, /* filter points to struct bpf_program */
        /* both receive and transmit directions */
        RTE_PDUMP_FLAG_RXTX = (RTE_PDUMP_FLAG_RX|RTE_PDUMP_FLAG_TX)
};




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] RFC - adding filter to packet capture API
  2019-12-09 13:41 ` Ananyev, Konstantin
@ 2019-12-09 16:49   ` Stephen Hemminger
  2019-12-11 20:13   ` Morten Brørup
  1 sibling, 0 replies; 5+ messages in thread
From: Stephen Hemminger @ 2019-12-09 16:49 UTC (permalink / raw)
  To: Ananyev, Konstantin; +Cc: Ray Kinsella, dev

On Mon, 9 Dec 2019 13:41:30 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> > 
> > 4. Keep existing function signature, but be type unsafe.
> >    This keeps API, but still is ABI breakage for programs that passed
> >    garbage. Plus C is unsafe enough already.
> >   
> 
> My preference is probably #4, with some extra changes:
> make actual type for 'filter' be determined by flags,
> something like:
> 
> enum {
>         RTE_PDUMP_FLAG_RX = 1,  /* receive direction */
>         RTE_PDUMP_FLAG_TX = 2,  /* transmit direction */
> +      RTE_PDUMP_FLAG_CBPF = 4, /* filter points to struct bpf_program */
>         /* both receive and transmit directions */
>         RTE_PDUMP_FLAG_RXTX = (RTE_PDUMP_FLAG_RX|RTE_PDUMP_FLAG_TX)
> };

Interesting but that is more awkward usage.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] RFC - adding filter to packet capture API
  2019-12-09 13:41 ` Ananyev, Konstantin
  2019-12-09 16:49   ` Stephen Hemminger
@ 2019-12-11 20:13   ` Morten Brørup
  1 sibling, 0 replies; 5+ messages in thread
From: Morten Brørup @ 2019-12-11 20:13 UTC (permalink / raw)
  To: Ananyev, Konstantin, Stephen Hemminger, Ray Kinsella; +Cc: dev

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Monday, December 9, 2019 2:42 PM 
> 
> > In the process of updating packet capture to take a filter program, there
> > is one open question about API/ABI.
> >
> > The example is:
> >
> > int
> > rte_pdump_enable(uint16_t port, uint16_t queue, uint32_t flags,
> > 		struct rte_ring *ring,
> > 		struct rte_mempool *mp,
> > 		void *filter);
> >
> > For the new version want to add ability to pass a BPF (classic) program
> > from libcap. This is described in most pcap API's as "struct
> bpf_program".
> >
> > The filter parameter was left as a placeholder, but in typical YAGNI
> > fashion. When we do need to use it, it is not going to work out.
> >
> > Since the existing filter argument was never used, there are a bunch
> > of options (in order from best to worse).
> >
> > 1. Introduce new API which takes a filter.
> >
> > int
> > rte_pdump_enable_bpf(uint16_t port, uint16_t queue, uint32_t flags,
> > 		struct rte_ring *ring,
> > 		struct rte_mempool *mp,
> > 		const struct bpf_program *filter);
> >
> > The old API is just the same as calling new API with NULL as filter.
> > This solution is safe but adds minor bloat.
> >
> > 2. Use API versioning.  This solves the ABI problem but it is still
> >    an API breakage since program that was passing junk would still
> >    not compile.
> >
> > 3. Change the function signature of existing API. This risks breaking
> >    at compile time some program that was passing some other value.
> >    Similarly, a program could have passed garbage, we never checked.
> >
> > 4. Keep existing function signature, but be type unsafe.
> >    This keeps API, but still is ABI breakage for programs that passed
> >    garbage. Plus C is unsafe enough already.
> >
> 
> My preference is probably #4, with some extra changes:
> make actual type for 'filter' be determined by flags,
> something like:
> 
> enum {
>         RTE_PDUMP_FLAG_RX = 1,  /* receive direction */
>         RTE_PDUMP_FLAG_TX = 2,  /* transmit direction */
> +      RTE_PDUMP_FLAG_CBPF = 4, /* filter points to struct bpf_program */
>         /* both receive and transmit directions */
>         RTE_PDUMP_FLAG_RXTX = (RTE_PDUMP_FLAG_RX|RTE_PDUMP_FLAG_TX)
> };
> 

I like Konstantin's idea of providing the filter type as a flag, as it still leaves the filter parameter open for other filter types in the future. It also allows using the existing pdump_request structure (and associated client/server infrastructure) as is.

And I appreciate very much that name of the flag explicitly indicates that the filter type is cBPF (not just BPF, which in librte_bpf actually means eBPF).
Did I mention that I hate the use of the name "BPF" instead of "eBPF", because "BPF" used to mean what is today also known as "cBPF"...


Med venlig hilsen / kind regards
- Morten Brørup


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-12-11 20:13 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 22:11 [dpdk-dev] RFC - adding filter to packet capture API Stephen Hemminger
2019-12-09 10:24 ` Ray Kinsella
2019-12-09 13:41 ` Ananyev, Konstantin
2019-12-09 16:49   ` Stephen Hemminger
2019-12-11 20:13   ` Morten Brørup

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).