On Sat, Feb 18, 2017 at 08:38:51AM +0100, Jiri Pirko wrote:
> Fri, Feb 17, 2017 at 09:49:07AM CET, simon.hor...@netronome.com wrote:
> >Hi Jiri,
> >
> >On Thu, Feb 16, 2017 at 04:22:37PM +0100, Jiri Pirko wrote:
> >> From: Arkadi Sharshevsky <arka...@mellanox.com>
> >>
> >> The pipeline debug is used to export the pipeline abstractions
> >> for the main objects - tables, headers and entries. The only support for
> >> set is for changing the counter parameter on specific table.
> >>
> >> The basic structures:
> >>
> >> Header - can represent a real protocol header information or internal
> >> metadata. Generic protocol headers like IPv4 can be shared
> >> between drivers. Each driver can add local headers.
> >>
> >> Field - part of a header. Can represent protocol field or specific
> >> ASIC metadata field. Hardware special metadata fields can
> >> be mapped to different resources, for example switch ASIC
> >> ports can have internal number which from the systems
> >> point of view is mapped to netdeivce ifindex.
> >>
> >> Hfield - Specific combination of header:field. This object is used
> >> to represent the table behavior in terms of match/action.
> >>
> >> Hfield_val - Represents value of specific Hfield.
> >>
> >> Table - represents a hardware block which can be described with
> >> match/action behavior. The match/action can be done on the
> >> packets data or on the internal metadata that it gathered
> >> along the packets traversal throw the pipeline which is vendor
> >> specific and should be exported in order to provide
> >> understanding of ASICs behavior.
> >>
> >> Entry - represents single record in a specific table. The entry is
> >> identified by specific combination of Hfield_vals for match
> >> /action.
> >>
> >> Prior to accessing the tables/entries the drivers provide the header/
> >> field data base which is used by driver to user-space. The data base
> >> is split between the shared headers and unique headers.
> >
> >Thanks for posting this. In general I think it looks quite promising.
> >
> >After a first pass over the code I have the following
> >specific comments:
> >
> >>
> >> Signed-off-by: Arkadi Sharshevsky <arka...@mellanox.com>
> >> Signed-off-by: Jiri Pirko <j...@mellanox.com>
> >> ---
> >> include/net/devlink.h | 224 ++++++++++++-
> >> include/uapi/linux/devlink.h | 50 ++-
> >> net/core/devlink.c | 747
> >> +++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 1019 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/net/devlink.h b/include/net/devlink.h
> >
> >...
> >
> >> +/**
> >> + * struct devlink_dpipe_entry - table entry object
> >> + * @index: index of the entry in the table
> >> + * @matches: tuple match values
> >> + * @matches_count: count of matches tuples
> >> + * @actions: tuple actions values
> >> + * @actions_count: count of actions values
> >> + * @counter: value of counter
> >> + * @counter_valid: Specify if value is valid from hardware
> >> + */
> >> +struct devlink_dpipe_entry {
> >> + unsigned int index;
> >
> >I'm not sure what I understand what index is but I assume that it is a
> >unique identifier for the flow within the table. From the point of view
>
> It is an index of the entry within the table, yes.
>
>
> >of having enough indexes for all entries in the table an unsigned int seems
> >adequate. But I see use-cases that have significantly wider identifiers.
> >
> >I'm wondering what your thoughts are on supporting wider identifiers.
>
> We can make this u64, no problem.
>
>
> >Perhaps they belong in the match?
> >
> >> + struct devlink_dpipe_hfield_val *matches;
> >> + unsigned int matches_count;
> >> + struct devlink_dpipe_hfield_val *actions;
> >> + unsigned int actions_count;
> >> + u64 counter;
> >
> >I'm unclear on what counter is. Is it the number of times the action entry
> >has been used (hit)? If so I think some provision for richer per-hit
>
> Yes.
>
> >counters for entries would be useful. At least number of hits and number of
> >bytes. But perhaps it would be useful to allow hardware to describe its
> >per-entry counters?
>
> Yeah, we were thinking about having this toggle per-entry. The thing is,
> you should be able to control per-entry over standard API. For example,
> for TCAM entry, you should use TC toggle to control counter on/off.
>
> This table-wide toggle is useful for debugging purposes of entries that
> are not exposed over standard API.
>
> Do you see a need to toggle this per-entry?
For the use-cases I am currently aware of I would not need a per-entry toggle.
> Thanks for the review!
>
> >
> >> + bool counter_valid;
> >> +};
> >
> >...
