Hi Alin,
Thanks for the comments. Will resend a patch with some of the updates.
Have responded to some of the comments inline.
Sairam
On 5/26/16, 3:00 AM, "Alin Serdean" <aserd...@cloudbasesolutions.com>
wrote:
>I am unsure if we actual need another file but that maybe a personal
>preference.
Sai: Ideally we should be able to re-use the code once it¹s in userspace.
With that idea, am trying to keep the file structure similar with
Daniele¹s patch.
>
>Other comments inlined.
>
>Alin.
>> +
>> +static const long long other_timeouts[] = {
>> + [OTHERS_FIRST] = 60 * 10000000,
>> + [OTHERS_MULTIPLE] = 60 * 10000000,
>> + [OTHERS_BIDIR] = 30 * 10000000,
>> +};
>[Alin Gabriel Serdean: ] Maybe define 10000000 somewhere and add a
>comment on how many ms/s they will be.
>> +
>> +static __inline struct conn_other*
>> +OvsCastConntrackEntryToOtherEntry(OVS_CT_ENTRY *conn) {
>> + return CONTAINER_OF(conn, struct conn_other, up); }
>[Alin Gabriel Serdean: ] Add an assert conn
>> +
>> +static __inline VOID
>> +OvsConntrackUpdateExpiration(struct conn_other *conn, long long now) {
>> + conn->up.expiration = now + other_timeouts[conn->state]; }
>[Alin Gabriel Serdean: ] Add an assert to conn
>> +
>> +enum ct_update_res
>> +OvsConntrackUpdateOtherEntry(OVS_CT_ENTRY *conn_,
>> + BOOLEAN reply,
>> + UINT64 now) {
>> + struct conn_other *conn = OvsCastConntrackEntryToOtherEntry(conn_);
>[Alin Gabriel Serdean: ] Add an assert to conn_
>> +
>> +OVS_CT_ENTRY *
>> +OvsConntrackCreateOtherEntry(UINT64 now) {
>> + struct conn_other *conn;
>> + conn = OvsAllocateMemoryWithTag(sizeof(struct conn_other),
>> + OVS_CT_POOL_TAG);
>> + /* XXX Handle memory allocation error*/
>[Alin Gabriel Serdean: ] Please handle the memory allocation or at least
>add an assert to it
Sai: I have a different update that covers it. Will add an assertion for
this patch.
>> + conn->up = (OVS_CT_ENTRY) {0};
>> + conn->state = OTHERS_FIRST;
>> + OvsConntrackUpdateExpiration(conn, now);
>> + return &conn->up;
>> +}
>> \ No newline at end of file
>[Alin Gabriel Serdean: ] Nit no new line
>> diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-
>> windows/ovsext/Conntrack.c
>> index 544fd51..e193e9f 100644
>> --- a/datapath-windows/ovsext/Conntrack.c
>> +++ b/datapath-windows/ovsext/Conntrack.c
>> @@ -146,9 +146,20 @@ OvsCtUpdateFlowKey(struct OvsFlowKey *key,
>> }
>> }
>>
>> +static __inline VOID
>> +OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx) {
>> + NdisMoveMemory(&entry->key, &ctx->key, sizeof (OVS_CT_KEY));
>> + NdisMoveMemory(&entry->rev_key, &ctx->key, sizeof (OVS_CT_KEY));
>[Alin Gabriel Serdean: ] Maybe use RtlCopyMemory
Sai: NdisMoveMemory is appropriate given that we are in NDIS context. It
calls RtlCopyMemory under the hoods.
#define NdisMoveMemory(Destination, Source, Length)
RtlCopyMemory(Destination, Source, Length)
>> + OvsCtKeyReverse(&entry->rev_key);
>> + InsertHeadList(&ovsConntrackTable[ctx->hash &
>> CT_HASH_TABLE_MASK],
>> + &entry->link);
>> +}
>> +
>> static __inline POVS_CT_ENTRY
>> -OvsCtEntryCreate(const TCPHdr *tcp,
>> - PNET_BUFFER_LIST curNbl,
>> +OvsCtEntryCreate(PNET_BUFFER_LIST curNbl,
>> + UINT8 ipProto,
>> + UINT32 l4Offset,
>> OvsConntrackKeyLookupCtx *ctx,
>> OvsFlowKey *key,
>> BOOLEAN commit,
>> @@ -156,26 +167,71 @@ OvsCtEntryCreate(const TCPHdr *tcp, {
>> POVS_CT_ENTRY entry = NULL;
>> UINT32 state = 0;
>> - if (!OvsConntrackValidateTcpPacket(tcp)) {
>> - state |= OVS_CS_F_INVALID;
>> - OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
>> - return entry;
>> - }
>> + switch (ipProto)
>> + {
>> + case IPPROTO_TCP:
>> + {
>> + TCPHdr tcpStorage;
>> + const TCPHdr *tcp;
>> + tcp = OvsGetTcp(curNbl, l4Offset, &tcpStorage);
>> + if (!OvsConntrackValidateTcpPacket(tcp)) {
>> + goto invalid;
>> + }
>> +
>> + state |= OVS_CS_F_NEW;
>> + if (commit) {
>> + entry = OvsConntrackCreateTcpEntry(tcp, curNbl,
>>currentTime);
>> + OvsCtAddEntry(entry, ctx);
>> + }
>> +
>> + OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
>> + return entry;
>> + }
>> + case IPPROTO_ICMP:
>> + case IPPROTO_UDP:
>> + state |= OVS_CS_F_NEW;
>> + if (commit) {
>> + entry = OvsConntrackCreateOtherEntry(currentTime);
>> + OvsCtAddEntry(entry, ctx);
>> + }
>>
>> - state |= OVS_CS_F_NEW;
>> - if (commit) {
>> - entry = OvsConntrackCreateTcpEntry(tcp, curNbl, currentTime);
>> - NdisMoveMemory(&entry->key, &ctx->key, sizeof (OVS_CT_KEY));
>> - NdisMoveMemory(&entry->rev_key, &ctx->key, sizeof
>>(OVS_CT_KEY));
>> - OvsCtKeyReverse(&entry->rev_key);
>> - InsertHeadList(&ovsConntrackTable[ctx->hash &
>> CT_HASH_TABLE_MASK],
>> - &entry->link);
>> + OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
>> + return entry;
>> + default:
>> + goto invalid;
>> }
>>
>> +invalid:
>> + state |= OVS_CS_F_INVALID;
>> OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
>> return entry;
>> }
>>
>> +static enum CT_UPDATE_RES
>> +OvsCtUpdateEntry(OVS_CT_ENTRY* entry,
>> + PNET_BUFFER_LIST nbl,
>> + UINT8 ipProto,
>> + UINT32 l4Offset,
>> + BOOLEAN reply,
>> + UINT64 now)
>> +{
>> + switch (ipProto)
>> + {
>> + case IPPROTO_TCP:
>> + {
>> + TCPHdr tcpStorage;
>> + const TCPHdr *tcp;
>> + tcp = OvsGetTcp(nbl, l4Offset, &tcpStorage);
>[Alin Gabriel Serdean: ] add in if (tcp) in the case it may be null
>> + return OvsConntrackUpdateTcpEntry(entry, tcp, nbl, reply,
>>now);
>> + }
>> + case IPPROTO_ICMP:
>> + case IPPROTO_UDP:
>> + return OvsConntrackUpdateOtherEntry(entry, reply, now);
>> + default:
>> + return CT_UPDATE_INVALID;
>> + }
>> +}
>> +
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
